Skip to content
This repository has been archived by the owner on Oct 23, 2022. It is now read-only.

fix(release 0.3.0: rewrite FsBlockStore.list; crate compiles as dependency again #464

Closed
wants to merge 1 commit into from

Conversation

Christian7573
Copy link

This PR rewrites FsBlockStore.list using for and while let loops, which removes a compilation error when using the ipfs crate as a dependency from another project. See issue #458 for relevant discussion.

Checklist (can be deleted from PR description once items are checked)

  • New code is “linted” i.e. code formatting via rustfmt and language idioms via clippy
  • There are no extraneous changes like formatting, line reordering, etc. Keep the patch sizes small!
  • There are functional and/or unit tests written, and they are passing
  • There is suitable documentation. In our case, this means:
    • Each command has a usage example and API specification
    • Rustdoc tests are passing on all code-level comments
    • Differences between Rust’s IPFS implementation and the Go or JS implementations are explained - N/A?
  • Additions to CHANGELOG.md files

@koivunej
Copy link
Collaborator

There are few fmt issues and I guess clippy as well. Sorry about that.

@koivunej
Copy link
Collaborator

It seems that there's something wrong with vcpkg.. Trying to find time to look into this.

@koivunej

This comment has been minimized.

@koivunej
Copy link
Collaborator

koivunej commented Aug 2, 2021

I got closer to the issue in #470 and deduced it was caused by a trouble inferring the two lifetimes generated by async-trait seen here in the expansion of the original code (with some tracing parts removed). Moving the method body as FsBlockStore::list0 outside the #[async_trait] impl BlockStore for FsBlockStore { ... } and just calling the new method in the BlockStore::list implementation worked around the issue.

I would prefer to keep the current "less buffering" version if possible, even if there's no concurrent execution of the streams. Could you peek at #470 and see if it matches your understanding of the compilation failure?

@Christian7573
Copy link
Author

#470 works for me. Awesome!

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

Successfully merging this pull request may close these issues.

2 participants