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

return err when rabin min less than 16 #3

Merged
merged 4 commits into from
Sep 7, 2018
Merged

Conversation

kjzz
Copy link
Contributor

@kjzz kjzz commented Sep 7, 2018

I want to fix bug in ipfs,so i need to update code in chunker.

@kjzz
Copy link
Contributor Author

kjzz commented Sep 7, 2018

Hey @Stebalien , the pr is used for ipfs issue,can you help me review it?Thx a lot

Copy link
Member

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Thanks!

parse.go Outdated
@@ -52,7 +54,9 @@ func parseRabinString(r io.Reader, chunker string) (Splitter, error) {
if err != nil {
return nil, err
}

if min < 16 {
return nil,ErrRabinMin
Copy link
Member

Choose a reason for hiding this comment

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

needs go fmt ./...

parse_test.go Outdated
}
_, err = parseRabinString(r, chk2)
if err == nil || err.Error() != ErrRabinMin.Error() {
t.Errorf("it should be a ErrRabinMin here.")
Copy link
Member

Choose a reason for hiding this comment

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

By convention error strings shouldn't end with a .

Copy link
Member

Choose a reason for hiding this comment

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

Also, you should be able to just do err == ErrRabinMin.

@kjzz
Copy link
Contributor Author

kjzz commented Sep 7, 2018

Hello @magik6k , i have update the pr.please help me review it.Thx a lot

In general, it's easier for people to parse positive statements.
@ghost ghost assigned Stebalien Sep 7, 2018
@ghost ghost added the status/in-progress In progress label Sep 7, 2018
@Stebalien Stebalien merged commit 5bf56e9 into ipfs:master Sep 7, 2018
@ghost ghost removed the status/in-progress In progress label Sep 7, 2018
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.

3 participants