Skip to content

Conversation

Trott
Copy link
Member

@Trott Trott commented Feb 9, 2017

Before: Missing expected exception..

Afer: Missing expected exception.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

assert

@Trott Trott added assert Issues and PRs related to the assert subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Feb 9, 2017
@nodejs-github-bot nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Feb 9, 2017
@Trott
Copy link
Member Author

Trott commented Feb 9, 2017

Technically semver-major because of the idiosyncratic way we have to be
super-careful with error messages. If anyone thinks it violates the
Locked API due to that, I won't argue the point. (Paging @ChALkeR!)

My thinking is that this doesn't violate the Locked API because it's a bug fix, even if we have a policy (at least at this time) to treat this particular type of bug fix like it's semver major.

But a "sorry, semver major is semver major, deal with it" approach also makes sense. (In which case, I might see if there's a general sentiment to unlock the API but that's another conversation.)

@gibfahn
Copy link
Member

gibfahn commented Feb 9, 2017

LGTM, and definitely seems related to #11200.

lib/assert.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this end with . when message is truthy?

(message ? ` ${message}.` : '');

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, unless message is expected to already end with ., so ¯\(ツ)/¯. And there should also be a : rather than . after expected if message is truthy.

Thinking about this some more and looking at the uses, I wonder if the thing to do is get rid of all the logic for appending . in the first place. Nothing else appends .. And this mostly only seems to do it wrong!

Current master results in messages like this:

AssertionError: Missing expected exception..
AssertionError: Missing expected exception. foo bar baz

wat?

Compare to assert.strictEqual() which does not append .:

AssertionError: 'a' === 'b'
AssertionError: foo bar baz

Copy link
Member Author

Choose a reason for hiding this comment

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

@lpinca OK, I just pushed some improvements based on your comment. PTAL

Before: `Missing expected exception..`

Afer: `Missing expected exception.`
@lpinca
Copy link
Member

lpinca commented Feb 10, 2017

@Trott

Thinking about this some more and looking at the uses, I wonder if the thing to do is get rid of all the logic for appending . in the first place.

This also makes sense imo, I think most error messages don't use . at all.

@Trott
Copy link
Member Author

Trott commented Feb 10, 2017

I'm going to apply the blocked label to this until #11304 lands or there's some other satisfactory resolution for #11200.

@Trott Trott added the blocked PRs that are blocked by other issues or PRs. label Feb 10, 2017
@jasnell
Copy link
Member

jasnell commented Feb 11, 2017

@Trott Trott removed the blocked PRs that are blocked by other issues or PRs. label Feb 15, 2017
@Trott
Copy link
Member Author

Trott commented Feb 15, 2017

assert is now Stable rather than Locked so this is un-blocked.

Trott added a commit to Trott/io.js that referenced this pull request Feb 15, 2017
Before: `Missing expected exception..`

Afer: `Missing expected exception.`

PR-URL: nodejs#11254
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@Trott
Copy link
Member Author

Trott commented Feb 15, 2017

Landed in 0af4183

@Trott Trott closed this Feb 15, 2017
@jasnell jasnell mentioned this pull request Apr 4, 2017
@Trott Trott deleted the fix-assert branch January 13, 2022 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. 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.

7 participants