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

tools: replace custom assert.fail lint rule #12287

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Apr 9, 2017

Replace custom lint rule for assert.fail() function signature errors
with a restricted-syntax rule.

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

tools

Replace custom lint rule for `assert.fail()` function signature errors
with a restricted-syntax rule.
@Trott Trott added the tools Issues and PRs related to the tools directory. label Apr 9, 2017
@Trott
Copy link
Member Author

Trott commented Apr 9, 2017

Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

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

Couple of questions

@@ -110,6 +110,9 @@ rules:
}, {
selector: "ThrowStatement > CallExpression[callee.name=/Error$/]",
message: "Use new keyword when throwing an Error."
}, {
selector: "CallExpression[callee.object.name='assert'][callee.property.name='fail'][arguments.length=1]",
Copy link
Member

@gibfahn gibfahn Apr 9, 2017

Choose a reason for hiding this comment

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

Is there a valid use-case for a two argument assert.fail?

Also if assert.fail(null, null, "message") is a valid use-case, we should probably document that.

Copy link
Member Author

@Trott Trott Apr 9, 2017

Choose a reason for hiding this comment

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

This came out of specific errors in the code base. I'm unaware of anyone ever misusing assert.fail() with two arguments in core, and once you are checking for that, the lint message you have to supply gets a lot trickier.

I don't like ramping up non-standard lint rules to be more strict to catch hypothetical problems because as you make the rules more and more strict, you end up generating false positives. But if we were to do that, I'd say let's do it in a different pull request. This one is replacing a custom rule with identical functionality using built-in ESLint capabilities. I'd rather not alter the functionality at the same time.

assert.fail() is an unfortunate API IMO. As far as I can tell, if you supply a truthy value for the third argument, the first two arguments and the fourth argument are all ignored. (I'm not looking at the code so maybe there's an edge case in there where that's not true. I doubt it, though.) And the first two arguments don't make a lot of sense without the fourth argument.

So....the useful signatures are assert.fail(value, value, falsy, value) or assert.fail(ignored, ignored, message).

It's a mess IMO but this lint rule was designed to catch just the one exceedingly common misuse.

Copy link
Member Author

Choose a reason for hiding this comment

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

By the way, PR incoming soon to make the API less confusing. :-D

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the detail, definitely not really related to this PR anyway.

Yeah, I find the idea that you have to have the third argument as falsy to actually use the fourth argument a bit hard to wrap my head around.

Copy link
Member Author

Choose a reason for hiding this comment

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

PR to make API less confusing: #12293

@@ -110,6 +110,9 @@ rules:
}, {
selector: "ThrowStatement > CallExpression[callee.name=/Error$/]",
message: "Use new keyword when throwing an Error."
}, {
selector: "CallExpression[callee.object.name='assert'][callee.property.name='fail'][arguments.length=1]",
message: "assert.fail() message should be third argument"
Copy link
Member

Choose a reason for hiding this comment

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

This is really clear if you did assert.fail('message'), but not if you made another mistake (i.e. did assert.fail(false) or something. Maybe something more generic like assert.fail() should take at least three arguments?

Copy link
Member Author

Choose a reason for hiding this comment

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

This rule came out of fixing a bunch of actual misuse in the code base. In every single case, it was assert.fail(string) so I'm pretty OK with it the way it is. I can't even fathom what someone would mean by assert.fail(false) to be honest.

Trott added a commit to Trott/io.js that referenced this pull request Apr 12, 2017
Replace custom lint rule for `assert.fail()` function signature errors
with a restricted-syntax rule.

PR-URL: nodejs#12287
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented Apr 12, 2017

Landed in b3f2e3b

@Trott Trott closed this Apr 12, 2017
@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
gibfahn pushed a commit that referenced this pull request Jun 18, 2017
Replace custom lint rule for `assert.fail()` function signature errors
with a restricted-syntax rule.

PR-URL: #12287
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
Replace custom lint rule for `assert.fail()` function signature errors
with a restricted-syntax rule.

PR-URL: #12287
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
Replace custom lint rule for `assert.fail()` function signature errors
with a restricted-syntax rule.

PR-URL: #12287
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
@Trott Trott deleted the assert-fail branch January 13, 2022 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants