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

implement CidFromReader #127

Merged
merged 1 commit into from
Jul 15, 2021
Merged

implement CidFromReader #127

merged 1 commit into from
Jul 15, 2021

Conversation

mvdan
Copy link
Contributor

@mvdan mvdan commented Jul 2, 2021

(see commit message)

@welcome

This comment has been minimized.

@mvdan mvdan mentioned this pull request Jul 2, 2021
@mvdan
Copy link
Contributor Author

mvdan commented Jul 2, 2021

Not super helpful that codecov is complaining about every uncovered line... I'd need to spend another 4h on this to get 100% coverage, and it feels somewhat futile.

Copy link

@dirkmc dirkmc left a comment

Choose a reason for hiding this comment

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

Logically the code LGTM so I'm approving 👍

I have one suggestion about how to refactor to make it easier to follow.

cid.go Outdated Show resolved Hide resolved
cid.go Outdated Show resolved Hide resolved
cid.go Outdated Show resolved Hide resolved
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

👌 very nice

Out of interest - is this style of return: (int, foo, error) where int is always meaningful even if error is non-nil an idiomatic Go thing? Would a user naturally assume that the first return value shouldn't be ignored in a non-fatal error? It's good that this is mentioned in the doc at least and I don't suppose there another way around this without a more sophisticated reader interface (like a pushback).

cid.go Show resolved Hide resolved
@warpfork
Copy link
Member

warpfork commented Jul 6, 2021

I'd love to have some tests that check that all the byte buffering dancing is correct. (It looks correct, but I've stopped trusting my eyes and mental model of the compiler completely in things as fiddly as this.)

Either would be good. The latter is more fragile and gives less holistic impact numbers, but also results in something that can actually go red/green in CI rather than requiring someone to keep an eyeball on things, so both have different kinds of value.

I don't know if I consider this a blocking request or not, though. If it's already been profiled from some place where it's being used, I'll believe it's correct. Having tests or benchmarks in the same repo will just make it easier to be sure we don't ever regress it.

@warpfork
Copy link
Member

warpfork commented Jul 6, 2021

Interface LGTM, anyway.

Returning an int like this to report partial progress in I/O reading is golang normative, yes. It describes a side effect that you couldn't otherwise get much insight into, and while that data's not always needed, when it is, you really need it, so it's good practice to always return it.

cid.go Outdated Show resolved Hide resolved
cid.go Outdated Show resolved Hide resolved
@mvdan
Copy link
Contributor Author

mvdan commented Jul 14, 2021

I don't know if I consider this a blocking request or not, though. If it's already been profiled from some place where it's being used, I'll believe it's correct. Having tests or benchmarks in the same repo will just make it easier to be sure we don't ever regress it.

This PR is pure functionality, not optimization/performance. It's virtually impossible to accomplish this without CidFromReader today, unless you're happy with reading a "probably large enough" chunk of bytes and hoping for the best.

In that way, I don't think performance should block this PR. I agree benchmarks and profiling would be nice, but that seems like it should come after. All I can say on the matter is that our use in carv2 works, and seems fast enough :)

I admit I didn't go out of my way to write more tests to try to get 100% test coverage on this func. I'll see if I can reuse TestBadCidFromBytes for that purpose.

@mvdan
Copy link
Contributor Author

mvdan commented Jul 14, 2021

The leading varint simplification that Steven suggested is now done.

I'll see if I can reuse TestBadCidFromBytes for that purpose.

That's now done too.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Some nits but otherwise lgtm.

cid.go Show resolved Hide resolved
cid.go Outdated Show resolved Hide resolved
cid.go Outdated Show resolved Hide resolved
cid.go Outdated Show resolved Hide resolved
And reuse two CidFromBytes tests for it, which includes both CIDv0 and
CIDv1 cases as inputs, as well as some inputs that should error.

Fixes #126.
@mvdan
Copy link
Contributor Author

mvdan commented Jul 14, 2021

@Stebalien the two byte slice comments are addressed, and the two perf nits now have TODOs. Want to take one last look?

@mvdan mvdan merged commit c4c8760 into master Jul 15, 2021
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.

6 participants