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

es6 modification and include error message #10016

Closed
wants to merge 1 commit into from

Conversation

christyleung1987
Copy link

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016
@imyller imyller added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 1, 2016
@mscdex mscdex added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 1, 2016

assert.throws(function() {
assert.throws(function(err, cb) {
if (err) return cb(err);
require('internal/freelist');
});
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, the snippet explaining the task probably should have been a bit more precise on what to do here – This function won’t be called with err and cb arguments.

assert.throws takes a second argument that can be a regexp and validates the error message against that (like this). Do you think you could update this PR with that?

Copy link
Author

Choose a reason for hiding this comment

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

ack, working on this now

Copy link
Member

Choose a reason for hiding this comment

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

Ping @christyleung1987: Still working on this? (If you have any questions or need additional assistance, feel free to ask here or on the #node-dev channel on Freenode IRC.)

@Trott
Copy link
Member

Trott commented Dec 24, 2016

@christyleung1987 I took the liberty of making the change requested by @addaleax and pushing it to your branch. Hope that's OK!

If some reviewers could please take a look, that would be great!

@Trott
Copy link
Member

Trott commented Dec 24, 2016

@christyleung1987
Copy link
Author

Hi all,

Sorry for the late reply, I didn't mean to ignore the messages, but I got the flu and strap throat at the same time and I'm still trying to recover from it. This has been a really great learning experience.

Trott pushed a commit to Trott/io.js that referenced this pull request Dec 24, 2016
* var -> const
* add RegExp to assert.throws() to check error message

PR-URL: nodejs#10016
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member

Trott commented Dec 24, 2016

Landed in c00f647.
Thanks for the contribution, @christyleung1987! 🎉

@Trott Trott closed this Dec 24, 2016
targos pushed a commit that referenced this pull request Dec 26, 2016
* var -> const
* add RegExp to assert.throws() to check error message

PR-URL: #10016
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 27, 2016
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
* var -> const
* add RegExp to assert.throws() to check error message

PR-URL: #10016
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
* var -> const
* add RegExp to assert.throws() to check error message

PR-URL: #10016
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. lib / src Issues and PRs related to general changes in the lib or src directory. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants