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

Properly check for valid argument Node in use-true-false rule #90

Merged
merged 2 commits into from Apr 29, 2016
Merged

Properly check for valid argument Node in use-true-false rule #90

merged 2 commits into from Apr 29, 2016

Conversation

bojand
Copy link
Contributor

@bojand bojand commented Apr 29, 2016

This fixes a crash in Atom using linter-xo that happens while typing on the opening bracket of t.truthy( or when closing on empty argument. This check makes sure the arg value is completely taken into account within the check before proceeding to check any of the || combinations. I was getting the crash in eslint-plugin-ava shipped with xo@0.14 which has different code than the master here; but I think the fix is still valid here.

This fixes a crash that happens while typing on the opening bracket of `t.truthy(` or when closing on empty argument. This check makes sure the arg is completely evaluated within the check before proceeding to check any of the `||` combinations.
@jfmengels
Copy link
Contributor

Ah yes, that makes total sense.

Would you mind adding new test cases for this? testCase('t.truthy()') and testCase('t.falsy()'). You can put them in the valid section I'd say (speaking of which, maybe we should a have a rule saying that assertions should always have arguments?)

Thanks a lot @bojand!

@jfmengels jfmengels added the bug label Apr 29, 2016
@bojand
Copy link
Contributor Author

bojand commented Apr 29, 2016

Added tests. Should have checked and done it from start.

Yea that may be a good idea to have something like no-empty-arg rule. You do mean like a separate rule altogether correct, and nothing related to this PR? Or specifically within context of this rule? Are there valid scenarios perhaps where someone would want such usage? I can't really think of it but I don't know. If it's a separate rule I imagine that can be a different PR altogether, its kind of outside of the scope of this.

@jfmengels
Copy link
Contributor

You do mean like a separate rule altogether correct, and nothing related to this PR?

Yes ;)

Are there valid scenarios perhaps where someone would want such usage?

Having an assertion without arguments? I don't think so no. Obviously there should be an exception for t.end(), t.pass() and t.fail(), but for the others, I don't see how it would be a valid use.

LGTM :)

@sindresorhus
Copy link
Member

Yea that may be a good idea to have something like no-empty-arg rule.

Instead, how about a rule that ensures the argument length is correct for each assertion?

So t.pass() would be allowed, t.pass('msg') too, but not t.pass('msg, 'foo').

@sindresorhus sindresorhus merged commit a45a7b2 into avajs:master Apr 29, 2016
@sindresorhus
Copy link
Member

Thank you @bojand :)

@jfmengels
Copy link
Contributor

eslint/eslint#6023 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants