errors: add duplicate symbol checking in E() and increase coverage - #11829
errors: add duplicate symbol checking in E() and increase coverage#11829DavidCai1111 wants to merge 1 commit into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
FWIW you could move the arguments to new (indented) lines to avoid new RegExp() and string concatenation:
assert.throws(
() => errors.E('TEST_ERROR_USED_SYMBOL'),
/^AssertionError: Error symbol: TEST_ERROR_USED_SYMBOL was used\.$/);
Sorry, something went wrong.
|
The first line of the commit message exceeds 50 characters. |
Sorry, something went wrong.
|
@mscdex Thanks for reviewing. Updated, PTAL. |
Sorry, something went wrong.
There was a problem hiding this comment.
The period should be escaped to avoid matching any character instead of a literal period.
Sorry, something went wrong.
There was a problem hiding this comment.
Oh...Sorry for forgetting that. Updated.
Sorry, something went wrong.
There was a problem hiding this comment.
maybe "was used before" or "was already used" ?
Sorry, something went wrong.
There was a problem hiding this comment.
@targos Thanks for reviewing. Changed to "was already used", PTAL.
Sorry, something went wrong.
Add duplicate symbol checking in E() to avoid potential confusing result. Increase coverage of internal/errors.
Sorry, something went wrong.
Add duplicate symbol checking in E() to avoid potential confusing result. Increase coverage of internal/errors. PR-URL: #11829 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Add duplicate symbol checking in E() to avoid potential confusing result. Increase coverage of internal/errors. PR-URL: nodejs#11829 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Add duplicate symbol checking in E() to avoid potential confusing result. Increase coverage of internal/errors. PR-URL: nodejs#11829 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
|
Doesn't land on v6.x afaict. If it does land somewhere else in the tree lmk |
Sorry, something went wrong.
Add duplicate error symbol checking in E() to avoid potential confusing result.
Improve coverage of internal/errors.js.
Checklist
Affected core subsystem(s)
errors