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.

Sign up for GitHub

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

Jump to bottom

src: fix space for module version mismatch error #10606

Closed
wants to merge 1 commit into from
Closed

src: fix space for module version mismatch error #10606

wants to merge 1 commit into from

Conversation

Kerumen
Copy link

@Kerumen Kerumen commented Jan 4, 2017 •
edited
Loading

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

CI: https://ci.nodejs.org/job/node-test-pull-request/5697/

@Kerumen
Copy link
Author

Kerumen commented Jan 4, 2017 •
edited
Loading

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.

Trott
Trott approved these changes Jan 4, 2017
View reviewed changes
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 •
edited
Loading

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

mhdawson
mhdawson approved these changes Jan 4, 2017
View reviewed changes
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 cjihrig mentioned this pull request Jan 5, 2017
Merged
2 tasks
@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