Skip to content
This repository has been archived by the owner on Jun 19, 2023. It is now read-only.

Use ipld.ErrNotFound #48

Merged
merged 2 commits into from
Mar 17, 2022
Merged

Use ipld.ErrNotFound #48

merged 2 commits into from
Mar 17, 2022

Conversation

hsanjuan
Copy link
Collaborator

@hsanjuan hsanjuan commented Apr 7, 2020

Rationale: ipfs/kubo#7074

@hsanjuan
Copy link
Collaborator Author

hsanjuan commented Apr 7, 2020

Note blockstore depends on raw-multihash-enabled datastore already, therefore this cannot be merged until that change is unlocked, or needs to be merged into a pre v1.0.0 branch.

@hsanjuan
Copy link
Collaborator Author

hsanjuan commented Mar 2, 2022

Pending ipfs/go-ipld-format#68

Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

LGTM

@BigLep BigLep requested review from Jorropo and removed request for guseggert and aschmahmann March 15, 2022 18:43
@Jorropo Jorropo merged commit b3ee1d9 into master Mar 17, 2022
@Jorropo Jorropo deleted the fix/error-types branch March 17, 2022 23:01
@orpheuslummis
Copy link

Note that this breaks code using blockstore.ErrNotFound.

@Jorropo
Copy link
Contributor

Jorropo commented Mar 28, 2022

@orpheuslummis yes, that why this code has been released with a new minor version (not patch). 🙂
1.2.0 aa05ad6

If you see ipfs/kubo#7074 (comment) you can find most of the related PRs that updates various go-ipfs modules.

@orpheuslummis
Copy link

Thanks for the reference to the PRs. :)
That said semver indicates that minor version bumps should be backwards compatible. I'm assuming that go-ipfs-blockstore follows semver as this is what go-ipfs is doing as indicated https://github.com/ipfs/go-ipfs/blob/master/docs/releases.md.

@Jorropo
Copy link
Contributor

Jorropo commented Mar 28, 2022

That said semver indicates that minor version bumps should be backwards compatible.

@orpheuslummis

We usually use 0.x versions which use patch for minors and minors for breaking instead.

Under 0.x bumping the minor communicate a breaking change.

This modules somehow, have a 1.x release which then you are right should be a major bump (I didn't realised, I thought it was 0.2.0 not 1.2.0, I guess @hsanjuan had the same thing when he made the release).

I'll raise this issue with the maintainer team, but my personal opinion is that we do a semver like where we do <major breaking change, complete API rethink>.<breaking change>.<fix / patch> and not semver.

@Jorropo
Copy link
Contributor

Jorropo commented Mar 28, 2022

@orpheuslummis a really good point has been raised by someone else.

Golang's way of importing V2 of packages require adding /v2 to the import path.

Having to bump all imports paths and not just go.mod files for such trivial change sounds like a useless suffering.

@orpheuslummis
Copy link

I agree. I suggest considering switching to using v0.x.x versioning to avoid future confusions and to be semver-compatible.

@Jorropo
Copy link
Contributor

Jorropo commented Mar 28, 2022

@orpheuslummis I don't like that idea, golang also plays very badly with that, every time people would go get it would attempt to download the 1.x release.

Btw this package got a 1.x release because at one point they needed two branches because of two projects that were doing things differently, so they maintained a 0.x branch and a 1.x branch.

@orpheuslummis
Copy link

Ah yes. Then I suggest indicating in the README the versioning detail of the repo because it diverges from semver and other go-ipfs projects.

@Jorropo
Copy link
Contributor

Jorropo commented Mar 30, 2022

@orpheuslummis thx for your report, this has been openned:

ipfs/kubo#8832

@willscott
Copy link

willscott commented Apr 25, 2022

could we in the short term put in an alias of

type ErrNotFound = ipld.ErrNotFound

@aschmahmann
Copy link
Contributor

The new version takes a parameter (the cid that wasn't found) so an alias is insufficient.

@willscott
Copy link

var ErrNotFound = ipld.ErrNotFound{Cid: cid.Undef}

@Jorropo
Copy link
Contributor

Jorropo commented Apr 25, 2022

@willscott

No because the main issue is that code that use blockstore.ErrNotFound usually intent to do:

err == blockstore.ErrNotFound

However that code is broken now because even if that the same error (what the checks want to do), your proposition would only match both identicaly errors and CIDs. (so an ipld.ErrNotFound with not cid.Undef wouldn't work).

@hsanjuan
Copy link
Collaborator Author

var ErrNotFound = ipld.ErrNotFound{Cid: cid.Undef}

iirc there is intent here, in that people should switch to ipld.IsNotFound() checkers etc.

@willscott
Copy link

Yeah, realized as posting that the blockstore approach doesn't work.

I think the unfortunate thing i'm hitting is that there are blockstore interactions with go-ipld-prime that had been built i think with an intention not to depend on ipld-format. I'm struggling to figure out what error to return now because i'm hesitant to introduce the ipld-format dependency in ipld-prime.

@Jorropo
Copy link
Contributor

Jorropo commented Apr 26, 2022

I'm struggling to figure out what error to return now because i'm hesitant to introduce the ipld-format dependency in ipld-prime.

I think there is a good argument to make about moving ErrNotFound in go-ipld-prime.

Then we can deprecate and add aliases in go-ipld-format.

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.

5 participants