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

fix UDP code #73

Merged
merged 2 commits into from
Nov 28, 2018
Merged

fix UDP code #73

merged 2 commits into from
Nov 28, 2018

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Nov 28, 2018

fixes #72

@ghost ghost assigned Stebalien Nov 28, 2018
@ghost ghost added the status/in-progress In progress label Nov 28, 2018
@richardschneider
Copy link

Why has the code changed from 17 to 273? Isn't this a major breaking change.

@Stebalien
Copy link
Member Author

This was a breaking change but the actual change happened in multiformats/multicodec#16. Unfortunately, the change was never propagated to js-multiaddr (or multiformats/multiaddr...). However, go-multiaddr was updated.

Copy link
Member

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

lgtm in general, no idea why CI is entirely broken

@vmx
Copy link
Member

vmx commented Nov 28, 2018

I agree that this is a breaking change. That information should be part of the commit message so that it ends up in the changelog (see the JS Coding Guidelines for more information).

Possible commit message:

fix: change UDP code

BREAKING CHANGE: The UDP code was changed in the multicodec table

The UDP code is now `273` instead of `17`. For the full discussion of this change
please see https://github.com/multiformats/multicodec/pull/16.

Fixes #17.

@daviddias
Copy link
Member

Adding a ping to @victorb to review as the Lead Maintainer of this module -- https://github.com/multiformats/js-multiaddr#lead-maintainer

Thanks for reviewing and pointing to the Contributing guidelines as a source on how to complete this PR, @vmx 👏🏽👏🏽👏🏽

@daviddias daviddias removed their request for review November 28, 2018 10:07
@victorb
Copy link
Member

victorb commented Nov 28, 2018

CI is failing here as the test-cases haven't been updated. See the failures here: https://ci.ipfs.team/blue/organizations/jenkins/Multiformats%2Fjs-multiaddr/detail/PR-73/1/tests

BREAKING CHANGE: The UDP code was changed in the multicodec table

The UDP code is now `273` instead of `17`. For the full discussion of this change
please see multiformats/multicodec#16.

Fixes #17.
@Stebalien
Copy link
Member Author

Seriously?

49:7 warning Unexpected use of continue statement no-continue
...
341:1 warning Unexpected 'todo' comment no-warning-comments

@victorb
Copy link
Member

victorb commented Nov 28, 2018

Great, thanks!

@victorb victorb merged commit 59d3663 into master Nov 28, 2018
@ghost ghost removed the status/in-progress In progress label Nov 28, 2018
@victorb victorb deleted the fix/72 branch November 28, 2018 22:38
@victorb
Copy link
Member

victorb commented Nov 28, 2018

@daviddias
Copy link
Member

@victorb did you completely ignore @vmx's indication? #73 (comment)

@Stebalien
Copy link
Member Author

@daviddias I fixed the commit message before this was merged.

@vmx
Copy link
Member

vmx commented Nov 29, 2018

@daviddias You have to look at the commit history and you'll find e8c3d7d, which contains the proper "breaking change" part (which also shows up in the changelog).

The GitHub message links to the merge commit, you would need to click through it's parents.

@daviddias
Copy link
Member

I see. I looked through the commit history but I looked at @victorb commits and not @Stebalien, my bad.

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.

UDP code is incorrect
6 participants