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

assert: fix assert.fail with zero arguments #13974

Merged
merged 1 commit into from
Jul 2, 2017

Conversation

BridgeAR
Copy link
Member

Use a default message in case there is no argument and set actual to undefined (it will show up as a property of the thrown error object). Currently the message would be undefined undefined undefined instead.

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

@nodejs-github-bot nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Jun 29, 2017
@refack refack added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 29, 2017
@refack
Copy link
Contributor

refack commented Jun 29, 2017

Defensive semver-major for error message change 😞

* `operator` {string} (default: '!=')

Throws an `AssertionError`. If `message` is falsy, the error message is set as
the values of `actual` and `expected` separated by the provided `operator`.
Otherwise, the error message is the value of `message`.
If you provide no arguments at all, a default message will be used instead.
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid using informal pronouns like you in the docs

Copy link
Member

Choose a reason for hiding this comment

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

Why is this a rule all of a sudden? I'd have written 'Without arguments, ...' but this hetze against 'you' seems misplaced.

Copy link
Member

Choose a reason for hiding this comment

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

It's not "all of a sudden". This has been the convention for quite some time.

Copy link
Member

Choose a reason for hiding this comment

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

As for why, there are many reasons...

Refs: http://www2.ivcc.edu/rambo/tip_formal_writing_voice.htm

Copy link
Member

Choose a reason for hiding this comment

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

If it helps at all, according to our style guide:

  • Generally avoid personal pronouns in reference documentation ("I", "you", "we").
    • Pronouns are acceptable in more colloquial documentation, like guides.
    • Use gender-neutral pronouns and mass nouns. Non-comprehensive examples:
      • OK: "they", "their", "them", "folks", "people", "developers", "cats"
      • NOT OK: "his", "hers", "him", "her", "guys", "dudes"

The style guide has had the "generally avoid" part since June 2015 (when it lived in the docs repo where its visibility was unfortunately limited). https://github.com/nodejs/docs/blob/753d262f1530b1aff3c7e46d3c116d810d1698d3/GUIDE.md

It does seem like a "why" for it might be beneficial in the style guide. Maybe @nodejs/documentation can sort out the best reasons. For me, seeing "you" in technical documentation makes me suspicious that it was written carelessly. I immediately wonder if its out of date, inaccurate, contains omissions, etc. That may or may not be a sensible reaction on my part, but there it is.

The other option, of course, would be to change the style guide to say we're perfectly fine with pronouns like "you" etc. in our technical docs.

Copy link
Member

Choose a reason for hiding this comment

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

I asked for this out loud on Twitter, and a protected account gave the answer of “good documentation informs the reader rather than instructing them”, which actually makes a lot of sense to me. It’s not like all usage of “you” violates that rule, but that it will tend to so seems reasonable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I personally think it's reasonable not to use you, I just tend to forget about it while writing something as I concentrate more on the code...
It was also not my intention to kick off a discussion about it.

Copy link
Member

Choose a reason for hiding this comment

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

“good documentation informs the reader rather than instructing them”

That sounds reasonable. As long as it doesn't devolve into endless reams of passive voice.

Copy link
Contributor

@refack refack Jun 30, 2017

Choose a reason for hiding this comment

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

Do we have a "professional" technical writer available? I'm sure between the bunch of us we have a 100man-year of writing experience, but sometimes having a professional go over everything, and also sprucing up the style guide is worth it...

[addition] Just an anecdote, because in Hebrew all verbs and nouns are gendered, the Ministry of education passed a recommendation that all STEM curriculum should be written in female tense, just as an equalizer 🤷‍♂️ (I think it's kind of nice)

@jasnell
Copy link
Member

jasnell commented Jun 30, 2017

@jasnell
Copy link
Member

jasnell commented Jun 30, 2017

CI failures appear to be unrelated.

@refack
Copy link
Contributor

refack commented Jun 30, 2017

CI failures appear to be unrelated.

#14015

PR-URL: nodejs#13974
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@refack
Copy link
Contributor

refack commented Jul 23, 2017

@nodejs/release @lpinca @jasnell @cjihrig @Trott
Originally I marked this semver major but in retrospect it could be considered a bug fix and semver-minor for the addition of "zero or single argument" assert.fail([message]).

AFAICT this PR has only this possibly breaking effect

diff --git a/test/parallel/test-assert-fail.js b/test/parallel/test-assert-fail.js
@@ -8,7 +8,7 @@ assert.throws(
   common.expectsError({
     code: 'ERR_ASSERTION',
     type: assert.AssertionError,
-    message: 'undefined undefined undefined'
+    message: 'Failed'
   })
 );

@addaleax just run a CitGM on a change that included this (https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/922/) and it's clean.

I'm reversing my position from #12293 (comment) mostly because there were several good and "safe" improvements to assert that are non trivial to port to v8.x, re-including this would save a lot of manual backporting which IMHO is more risky than landing this.

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.

10 participants