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

Code Review v0.4.4...v0.4.5 #3534

Closed
ghost opened this issue Dec 23, 2016 · 19 comments
Closed

Code Review v0.4.4...v0.4.5 #3534

ghost opened this issue Dec 23, 2016 · 19 comments
Assignees
Labels
need/review Needs a review
Milestone

Comments

@ghost
Copy link

ghost commented Dec 23, 2016

Reflects c99a82d

Notable changes

changes

improvements

bitswap

fixes

general changes and refactorings

bsd

metrics

misc

deps

testing

docs

@ghost ghost added the need/review Needs a review label Dec 23, 2016
@ghost ghost added this to the ipfs 0.4.5 milestone Dec 23, 2016
@ghost ghost assigned jbenet Dec 23, 2016
@whyrusleeping
Copy link
Member

whyrusleeping commented Dec 24, 2016

Some notes i took while going through and reviewing all this:

  • unixfs/format_test.go, 7b16a8d, TestMetedata name typo
  • 58a2c5f, no test for invalid routing option error message
  • 38703a8, unnecessarily changed formatting in proto file
  • 650eeee, not sure we want to completely silence gx install output
  • 805b504, 'ipfs key list' should also list key hash ID (perhaps with an option)
  • core/commands/publish.go, should use path.ParsePath instead of simply casting input to path.Path
  • e76b14a, looks like @Kubuxu manually edited the package.json resulting in weird formatting (Was later fixed)
  • 1b3158f, a few trailing spaces in t0180-pubsub.sh
  • 8ee4280, potentially unnecessary change in generated protobuf struct tags adding 'name=XYZ' fields
  • ba383ed, trailing whitespace, @kevina do you have go-fmt on save setup?
  • 1fd99da, unixfs/io/pbdagreader.go, in default case of NewPBFileReader I would prefer to expand the error checking out
  • 1e3b4ae, default os check case leaves a weird trailing message, also no tests for empty multiaddr (protocols == 0) thing
  • 43320e1, would prefer the defer'ed dr.Close() happens after the switch, not a big deal though
  • 396c629, need more tests around Tree and Copy methods for protonode
  • aae9eb7, weird that merge commit has changes in it...
  • c618698, would be nice to have a test that reproduces the issue this revert fixes (maybe using curl?)
  • c31e4f7, not sure if we have tests for the context cancelling in the gateway. We should make sure to add these once this gets extracted
  • 4a029c7, need tests for swarm peers verbose mode
  • bfe7ad7, should probably add a test here that lists links of a raw object that we don't have
  • 02aa6b4, should no longer need null import. This is a holdover from Godeps aggressively removing things
    • lgierth: this null import exists so that gx doesn't forget this dependency. has happened twice in the past :)
  • b78b29c, tests for routing resolver failure cases might be nice. Do we have a good mock routing system?
  • 8ce9963, did i purposely remove an apostrophe from a comment in a sharness test?
  • 48f7e14, should add a test for 'ipfs ls' on a dir with raw nodes in it
  • 01aee44, in the future, do name changes in one commit, and restructuring in another
  • 1173c00, in AddBlocks, append to array 'if !has' to avoid using extra flow control structures
  • Aside: we should think about separating 'providing' from 'giving a block to bitswap'. This will make it easier to address the providers problem moving forward
  • f4d7369, should have caught the 'not sending fullness in wantlists' bug here, need better CR
  • Note: when parsing a CID fails, we should return a better error message than 'unrecognized multibase encoding' or whatever it is we're spewing now
  • cb3bda7, oh boy. This one got lots of review i remember. Still worth everyone taking another look at it
  • bf80213, we still need to get around to refactoring the 'node state' things, offline, epehemeral, etc

@whyrusleeping
Copy link
Member

Important to look at for @jbenet are the bitswap message sender change (to support cids, bitswap 1.1.0 and 1.0.0) and the merkledag dagservice changes to support cbor nodes, raw nodes, and the original protobufs

@kevina
Copy link
Contributor

kevina commented Jan 4, 2017

@whyrusleeping, if you want go through and create pull requests to fix most of these issues, let me know.

@kevina
Copy link
Contributor

kevina commented Jan 4, 2017

8ce9963, did i purposely remove an apostrophe from a comment in a sharness test?

yes, and I even mentioned it to you, something about about it causing problems with your editor :)

@whyrusleeping
Copy link
Member

I'm a dolt

@kevina
Copy link
Contributor

kevina commented Jan 4, 2017

I'm a dolt

Do you want me to fix it?

@whyrusleeping
Copy link
Member

That might be a very difficult problem to fix, you can try.

@kevina
Copy link
Contributor

kevina commented Jan 4, 2017

@whyrusleeping, I meant add back the apostrophe. :) I am fixing minor formatting/spelling issues now.

@kevina
Copy link
Contributor

kevina commented Jan 4, 2017

38703a8, unnecessarily changed formatting in proto file

don't see the problem other than diff noise, the changes help with consistency

unixfs/format_test.go, 7b16a8d, TestMetedata name typo
1b3158f, a few trailing spaces in t0180-pubsub.sh
8ce9963, did i purposely remove an apostrophe from a comment in a sharness test?

fixed in pr #3562

ba383ed, trailing whitespace, @kevina do you have go-fmt on save setup?

Already fixed, sorry.

8ee4280, potentially unnecessary change in generated protobuf struct tags adding 'name=XYZ' fields

this was talked about in the issue

@kevina
Copy link
Contributor

kevina commented Jan 5, 2017

I edited @whyrusleeping to be check boxes and checked resolved problems are ones where there is nothing to do. Github has no edit history and also there is no notification sent when an edit is made so I will continue to add notes on items in separate comments.

@kevina
Copy link
Contributor

kevina commented Jan 5, 2017

48f7e14, should add a test for 'ipfs ls' on a dir with raw nodes in it

Now fixed.

bfe7ad7, should probably add a test here that lists links of a raw object that we don't have

Your right. There is an issue for that.

@Kubuxu
Copy link
Member

Kubuxu commented Jan 10, 2017

bfe7ad7, should probably add a test here that lists links of a raw object that we don't have

in #3583

58a2c5f, no test for invalid routing option error message

in #3584

43320e1, would prefer the defer'ed dr.Close() happens after the switch, not a big deal though

in #3585

e76b14a, looks like @Kubuxu manually edited the package.json resulting in weird formatting (Was later fixed)

Probably while resolving merge conflict

@Kubuxu
Copy link
Member

Kubuxu commented Jan 11, 2017

43320e1, would prefer the defer'ed dr.Close() happens after the switch, not a big deal though

The close has to be where it is, due to one more error code that continues function flow but returns nil DagReader.

@kevina
Copy link
Contributor

kevina commented Jan 11, 2017

805b504, 'ipfs key list' should also list key hash ID (perhaps with an option)

#3581

@kevina
Copy link
Contributor

kevina commented Jan 11, 2017

1173c00, in AddBlocks, append to array 'if !has' to avoid using extra flow control structures

#3590

@kevina
Copy link
Contributor

kevina commented Jan 12, 2017

I have checked off all points that are either already addressed or are in an open p.r. I will look into the following remaining issues:

core/commands/publish.go, should use path.ParsePath instead of simply casting input to path.Path
1fd99da, unixfs/io/pbdagreader.go, in default case of NewPBFileReader I would prefer to expand the error checking out
1e3b4ae, default os check case leaves a weird trailing message, also no tests for empty multiaddr (protocols == 0) thing

What remains are some test, I might look into those after I address the above, but they are also testing parts of the code I am not familiar with so I am not sure how qualified I am to write them.

@kevina
Copy link
Contributor

kevina commented Jan 12, 2017

core/commands/publish.go, should use path.ParsePath instead of simply casting input to path.Path

#3592

1fd99da, unixfs/io/pbdagreader.go, in default case of NewPBFileReader I would prefer to expand the error checking out

I am unsure what you are after here.

1e3b4ae, default os check case leaves a weird trailing message, also no tests for empty multiaddr (protocols == 0) thing

Fixed the first part in #3591. Not 100% sure what you are after in the second part.

@ghost
Copy link
Author

ghost commented Jan 27, 2017

I updated the list in the top comment -- 22 items added, for a total of now 199 changes. added:

misc

deps

ipld

docs

general

fixes

testing

features

@whyrusleeping
Copy link
Member

closing this now

@whyrusleeping whyrusleeping removed the status/ready Ready to be worked label Feb 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review
Projects
None yet
Development

No branches or pull requests

4 participants