Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

ls_with_options #61

Merged
merged 2 commits into from
Nov 2, 2020
Merged

ls_with_options #61

merged 2 commits into from
Nov 2, 2020

Conversation

jcaesar
Copy link
Contributor

@jcaesar jcaesar commented Oct 24, 2020

This adds options to ipfs ls in the style of #54.

In theory, this should be a no-brainer. In practice, there's a small detail: the path parameter to ls is not actually optional, I don't know why it's an Option on IpfsClient::ls, and I'd prefer not to replicate that mistake. I've made it so the API doesn't break. If you want me to break IpfsClient::ls for consistency and correctnesses sake, please tell me.

(Btw, these are rather necessary to list large folders: ipfs ls --stream --resolve-type=false --size=false /ipfs/bafybeicupvauz2urhyg7brbqy37z6ytorxmsog7uns6scntmd3cpvxfnyu/distfiles should complete in about 20 minutes. Remove --stream and you just get no output for a very long time (but it does compelete). Remove the other two, too, and my node crashes. Though admittedly, folders with 10000s of items are rare.)

@ferristseng
Copy link
Owner

In practice, there's a small detail: the path parameter to ls is not actually optional, I don't know why it's an Option on IpfsClient::ls, and I'd prefer not to replicate that mistake. I've made it so the API doesn't break. If you want me to break IpfsClient::ls for consistency and correctnesses sake, please tell me.

Thanks for catching that mistake. I think just breaking the API is fine here for correctness sake.

Otherwise, looks great to me! I'll merge it when you make that change.

@jcaesar
Copy link
Contributor Author

jcaesar commented Nov 1, 2020

I think just breaking the API is fine here for correctness sake.

Done. (I wonder if it's ok to have api-breaking changes between two RCs, but probably wouldn't mind even if it wasn't ok.)

@ferristseng
Copy link
Owner

ferristseng commented Nov 2, 2020

I think just breaking the API is fine here for correctness sake.

Done.

Excellent, thank you!

(I wonder if it's ok to have api-breaking changes between two RCs, but probably wouldn't mind even if it wasn't ok.)

The version-ing is kind of a mess right now, and that's my fault. I think I'm just going to go back to minor versions and not mess around with RCs. It sounds like public API changes are fine in pre 1.0 releases regardless: https://semver.org/#spec-item-4

@ferristseng ferristseng merged commit c2b10b6 into ferristseng:master Nov 2, 2020
@jcaesar
Copy link
Contributor Author

jcaesar commented Nov 2, 2020

TIL. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants