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

Initial version of ipfs-pump #1

Merged
merged 13 commits into from
Jul 4, 2019
Merged

Initial version of ipfs-pump #1

merged 13 commits into from
Jul 4, 2019

Conversation

MichaelMure
Copy link
Collaborator

@MichaelMure MichaelMure commented Jun 21, 2019

This PR introduce a first version of ipfs-pump that support:

  • multiple source/destination: API, FlatFS, S3
  • parallel processing
  • a cute progress bar

@MichaelMure MichaelMure marked this pull request as ready for review June 25, 2019 19:30
@MichaelMure
Copy link
Collaborator Author

The money shot 🎉 :

image

Copy link
Contributor

@shazow shazow left a comment

Choose a reason for hiding this comment

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

Pump It Up!

LICENSE Outdated
@@ -0,0 +1 @@
Just a note, some code has been lifted from go-ipfs (MIT)
Copy link
Contributor

Choose a reason for hiding this comment

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

To be more official about this, put it in a NOTICE file with a copypasta of go-ipfs's license verbatim. Bonus points if you describe the scope of the copied code. Personally I like to keep it isolated (e.g. in its own .go file) so that it's easy to remove/replace later.

If the borrowed piece is really small, I sometimes just put it inline. E.g. this one function (which I plan to remove eventually): https://github.com/vipnode/vipnode/blob/master/jsonrpc2/borrowed_eth.go

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it's actually even less than that. Basically I went to look at the go-ipfs source to see how the components are articulated as there is pretty much no documentation about the internals. I can see where it has been useful, but there is pretty nothing that I can actually call "borrowed code".

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, I wouldn't even bother mentioning it here but just give it a one-liner comment in the relevant place "Used X as reference to implement" kinda thing.

enumArg = kingpin.Arg("enum", "The source to enumerate the content. "+
"Possible values are ["+strings.Join(enumValues, ",")+"].").
Required().Enum(enumValues...)
collValues = []string{CollAPI, CollFlatFS, CollS3}
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW: Saw a bunch of "coll" prefixes, took me a long time to figure out it was "collector" abbreviated. :P

Copy link
Collaborator Author

@MichaelMure MichaelMure Jun 25, 2019

Choose a reason for hiding this comment

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

do you have someting more meaningful in mind ? It kind of make sense for me, but as english is not my first language sometimes I miss some stuff (and you are all very much welcome to help/correct me about it).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd default to spelling out "collector" or "collect". It's not that much longer.

Coll just threw me off because it's so close to "Call" which I see all the time. It's just me though, so don't feel pressured to make the change if it looks fine to you. :)

@shazow shazow requested a review from a team June 25, 2019 19:53
@MichaelMure
Copy link
Collaborator Author

@johanherman I'm curious to see how this behave with cluster !

@johanherman
Copy link

@MichaelMure Weee! Will have a look!

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.

3 participants