Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(command): merge file ls and ls #5500

Closed
wants to merge 3 commits into from

Conversation

overbool
Copy link
Contributor

@overbool overbool commented Sep 20, 2018

License: MIT
Signed-off-by: Overbool overbool.xu@gmail.com

Fixes: #5497 and #5502

@overbool overbool requested a review from Kubuxu as a code owner September 20, 2018 14:27
@overbool overbool force-pushed the fix/merge-file-ls-and-ls branch 2 times, most recently from a254b9e to 17a3f9a Compare September 21, 2018 00:33
> ipfs file ls QmW2WQi7j6c7UgJTarActp7tDNikE4B2qXtFCfLPdsgaTQ
cat.jpg
> ipfs file ls /ipfs/QmW2WQi7j6c7UgJTarActp7tDNikE4B2qXtFCfLPdsgaTQ
cat.jpg
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this information is overkill. Syntax for paths should be documented somewhere else.

@@ -26,7 +26,7 @@ import (
type LsLink struct {
Name, Hash string
Size uint64
Type unixfspb.Data_DataType
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I like the idea of replacing an enum type with a string....

@overbool
Copy link
Contributor Author

@kevina Thx for your reply, i am planning to block this pr until #5217 is fixed.

@overbool overbool force-pushed the fix/merge-file-ls-and-ls branch 5 times, most recently from d014e21 to 1648961 Compare September 24, 2018 12:36
@overbool overbool force-pushed the fix/merge-file-ls-and-ls branch from 1648961 to 34ebe30 Compare October 5, 2018 03:59
@overbool overbool changed the title [WIP] feat(command): merge file ls and ls feat(command): merge file ls and ls Oct 5, 2018
@overbool overbool force-pushed the fix/merge-file-ls-and-ls branch 2 times, most recently from 1e00655 to 3ea54c3 Compare October 5, 2018 05:47
@Kubuxu
Copy link
Member

Kubuxu commented Oct 9, 2018

We have to maintain API compatibilities so we can only add fields.

@overbool
Copy link
Contributor Author

We have to maintain API compatibilities so we can only add fields.

@Kubuxu Which modification do you mean?

@Kubuxu
Copy link
Member

Kubuxu commented Oct 10, 2018

As an example the switch from enum to string.

@overbool overbool force-pushed the fix/merge-file-ls-and-ls branch from 3ea54c3 to d4c1d19 Compare October 10, 2018 11:20
License: MIT
Signed-off-by: Overbool <overbool.xu@gmail.com>
@overbool overbool force-pushed the fix/merge-file-ls-and-ls branch from d4c1d19 to dc9456f Compare October 10, 2018 11:47
License: MIT
Signed-off-by: Overbool <overbool.xu@gmail.com>
License: MIT
Signed-off-by: Overbool <overbool.xu@gmail.com>
@overbool overbool force-pushed the fix/merge-file-ls-and-ls branch from cf54426 to c36b563 Compare October 10, 2018 12:42
@overbool
Copy link
Contributor Author

@Kubuxu I had changed it as your suggestion and updated the test.

@guseggert guseggert closed this Sep 15, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge 'file ls' and 'ls' commands
4 participants