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.

Sign up for GitHub

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

Jump to bottom

tools: replace custom assert.fail lint rule #12287

Closed
wants to merge 1 commit into from
Closed

tools: replace custom assert.fail lint rule #12287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .eslintrc.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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

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.

}]
no-tabs: 2
no-trailing-spaces: 2
Expand Down Expand Up @@ -142,7 +145,6 @@ rules:

# Custom rules in tools/eslint-rules
align-multiline-assignment: 2
assert-fail-single-argument: 2
assert-throws-arguments: [2, { requireTwo: false }]
no-unescaped-regexp-dot: 2

Expand Down
30 changes: 0 additions & 30 deletions tools/eslint-rules/assert-fail-single-argument.js
View file
Open in desktop

This file was deleted.