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

Tweak parameters of NewPrefix functions. #51

Closed
wants to merge 1 commit into from
Closed

Conversation

kevina
Copy link
Contributor

@kevina kevina commented Jun 25, 2018

I don't think these two functions are really used yet, so this shouldn't break anything.

NewPrefixV0 doesn't take any parameters, since there is only one possible legal combination. NewCidV1 is expanded to also take the multihash length. Even though the length 99% of the time will be -1 it still makes sense to require it. This gives the function the opportunity to convert the length value of -1 to the default length. The callee of the function is already likely to have the length value and needs to do something with it. With out being able to pass it in the most logical think to do would be to set it afterwards, for example:

  func foo(..., mh uint64, mhlen int) { 
    prefix := cid.NewPrefixV1(..., mh)
    prefix.MhLength = mhlen
  }

which doesn't allow NewPrefix to set the value to the default length.

Closes #49.

@kevina kevina requested a review from Stebalien June 25, 2018 03:33
@ghost ghost assigned kevina Jun 25, 2018
@ghost ghost added the status/in-progress In progress label Jun 25, 2018
@kevina
Copy link
Contributor Author

kevina commented Jun 27, 2018

@Stebalien do you think we could get this change in ASAP? Right now it doesn't look like these functions are used. Once they become used this change will become more difficult.

@Stebalien
Copy link
Member

So, aren't we planning on replacing these functions with a Format struct and a NewFormat function?

@Stebalien
Copy link
Member

That is, I'd rather deprecate and remove these functions and move to that than to break things.

Stebalien
Stebalien previously approved these changes Jun 28, 2018
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.

At the moment, this'll only break a few tests and I agree it's more correct. However, I'd rather move forward with the Format constructor (complete with options) and simply deprecate these, if possible.

@Stebalien
Copy link
Member

Possible way forward:

  1. Merge add some helpers for creating new CIDs #52 first.
  2. Make everything that uses these functions use those.
  3. Deprecate these functions.
  4. Remove at our leisure.

Basically, changing the signature will be the same amount of work as deprecating and removing.

@kevina
Copy link
Contributor Author

kevina commented Jun 28, 2018

@Stebalien

Not merging this P.R. is partly blocking #50.

A grep of the entire source tree found that these functions are used in only one place outside of go-cid in go-libp2p-routing-helpers in parallel_test.go.

#52 will take awhile to review and get in, this is a very simple change.

My plan was to get #40/#50 in before #52.

As far work, all that needs to be done is to just merge this P.R. We don't need to GX publish just yet. If this gets GX published before #52 gets in fixing the signature in the one place is not a lot of work compared to the updating all the places that use go-cid.

@Stebalien
Copy link
Member

As far work, all that needs to be done is to just merge this P.R. We don't need to GX publish just yet. If this gets GX published before #52 gets in fixing the signature in the one place is not a lot of work compared to the updating all the places that use go-cid.

That'll still break anyone calling go test ./.... While we use gx, we try not to rely on it.

Also, there are other users:

  • go-ipfs-blockstore
  • Bit-Nation/panthalassa
  • mgoelzer/libp2p-chat-app
  • mikiquantum/rock-paper-scisors-demo
  • ...

If an interface is broken and we'll want to break it eventually, I'm usually all for breaking it sooner rather than later. However, given that we want to simply replace these functions, I'd rather leave them as-is (they still work, even if they allow invalid states) and then replace and deprecate them.

@Stebalien Stebalien dismissed their stale review June 28, 2018 17:40

Found a bunch of non-PL projects that rely on this.

@kevina
Copy link
Contributor Author

kevina commented Aug 9, 2018

Closing in favor of #53.

@kevina kevina closed this Aug 9, 2018
@ghost ghost removed the status/in-progress In progress label Aug 9, 2018
@mvdan mvdan deleted the kevina/newprefix branch July 1, 2021 16:13
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.

NewPrefixV0 should not take a multihash
2 participants