Skip to content

fix UDP code - #73

Merged
victorb merged 2 commits into
masterfrom
fix/72
Nov 28, 2018
Merged

fix UDP code#73
victorb merged 2 commits into
masterfrom
fix/72

Conversation

@Stebalien

@Stebalien Stebalien commented Nov 28, 2018

Copy link
Copy Markdown
Member

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
Copy Markdown

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

@Stebalien

Copy link
Copy Markdown
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.

@dignifiedquire dignifiedquire left a comment

Copy link
Copy Markdown
Member

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

vmx commented Nov 28, 2018

Copy link
Copy Markdown
Member

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
daviddias requested a review from victorb November 28, 2018 10:05
@daviddias

Copy link
Copy Markdown
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

victorb commented Nov 28, 2018

Copy link
Copy Markdown
Member

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
Copy Markdown
Member Author

Seriously?

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

@victorb

victorb commented Nov 28, 2018

Copy link
Copy Markdown
Member

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

victorb commented Nov 28, 2018

Copy link
Copy Markdown
Member

@daviddias

Copy link
Copy Markdown
Member

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

@Stebalien

Copy link
Copy Markdown
Member Author

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

@vmx

vmx commented Nov 29, 2018

Copy link
Copy Markdown
Member

@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
Copy Markdown
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