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

fix: add dag walker for json codec #247

Merged
merged 1 commit into from
Sep 2, 2023
Merged

Conversation

achingbrain
Copy link
Member

Since the json codec is bundled with multiformats we should add a dag walker for it to allow pinning json blocks.

Fixes #246

Since the json codec is bundled with multiformats we should add a
dag walker for it to allow pinning json blocks.

Fixes #246
Copy link
Contributor

@whizzzkid whizzzkid left a comment

Choose a reason for hiding this comment

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

Lgtm with nits

expect(pins).to.have.nested.property('[0].metadata').that.eql({})

await expect(helia.pins.isPinned(cid1)).to.eventually.be.true()
await expect(helia.pins.isPinned(cid2)).to.eventually.be.true()
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests are the same except the data being encoded and pinned. This can be refactored to run as a loop over different encoded values. I feel it makes the test more readable as it would be immediately clear what exactly the test is testing.

Optional.

@achingbrain achingbrain merged commit 5c4b570 into main Sep 2, 2023
15 checks passed
@achingbrain achingbrain deleted the fix/add-json-dag-walker branch September 2, 2023 14:17
This was referenced Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Pinning doesn't work with @helia/json
3 participants