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

src: fix space for module version mismatch error #10606

Closed

Conversation

Kerumen
Copy link

@Kerumen Kerumen commented Jan 4, 2017

The Native Module version mismatch error was missing a space at the end, between or and npm install:

screen shot 2017-01-04 at 14 36 59

Checklist
  • make -j4 test (UNIX)
  • commit message follows commit guidelines
Affected core subsystem(s)

src

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 4, 2017
@addaleax addaleax added addons Issues and PRs related to native addons. semver-major PRs that contain breaking changes and should be released in the next major version. labels Jan 4, 2017
@JacksonTian
Copy link
Contributor

@Kerumen
Copy link
Author

Kerumen commented Jan 4, 2017

Why is this labeled semver-major?

@cjihrig
Copy link
Contributor

cjihrig commented Jan 4, 2017

All changes to error messages are considered semver major.

@Trott
Copy link
Member

Trott commented Jan 4, 2017

Can this also update test/addons/node-module-version/test.js to check that part of the error message? Currently, it has a regex that stops before the last sentence.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 4, 2017

I was going to PR this after this one landed. Here is the test update commit: cjihrig@8283bfa

@Trott
Copy link
Member

Trott commented Jan 4, 2017

(@Kerumen By the way, if you don't want to update the test--if you were hoping to just add a space and move on--I can fix up the test. But if you're up for it, go for it.)

@Trott
Copy link
Member

Trott commented Jan 4, 2017

Oh, never mind, @cjihrig has it covered. Sorry everyone.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM

@Kerumen
Copy link
Author

Kerumen commented Jan 4, 2017

@Trott I could totally do it. Didn't know it was needed.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@cjihrig
Copy link
Contributor

cjihrig commented Jan 5, 2017

One test failed in the CI, but it was unrelated. This is good to land at 9AM Eastern tomorrow.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 6, 2017

Landed in 334be0f. Thanks!

@cjihrig cjihrig closed this Jan 6, 2017
cjihrig pushed a commit that referenced this pull request Jan 6, 2017
PR-URL: #10606
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@Kerumen Kerumen deleted the fix_node_module_version_message branch January 6, 2017 15:27
cjihrig added a commit to cjihrig/node that referenced this pull request Jan 9, 2017
Refs: nodejs#10606
PR-URL: nodejs#10636
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
PR-URL: nodejs#10606
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
@jasnell jasnell mentioned this pull request Apr 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addons Issues and PRs related to native addons. c++ Issues and PRs that require attention from people who are familiar with C++. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants