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

Add assertion-arguments rule (fixes #92) #95

Merged
merged 1 commit into from
May 4, 2016

Conversation

jfmengels
Copy link
Contributor

@jfmengels jfmengels commented May 1, 2016

Add assertion-arguments rule (fixes #92)

Willing to change the name of the rule, but this is pretty generic, and maybe some day we'll want to detect the argument types.

Probably to be followed by the deprecation of assertion-message.


## Options

The rule takes one option, a string, which could be either `"always"` (enforces error messages) or `"never"` (forbids assertion messages). If omitted or set to false, assertion messages will be neither enforced nor forbidden.
Copy link
Member

Choose a reason for hiding this comment

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

It no longer makes sense to have this as a main boolean option. I think it should rather be part of an options object.

@sindresorhus
Copy link
Member

sindresorhus commented May 2, 2016

Maybe we should also ensure the message argument is a string? I know you could potentially put it in an external string, but most put it inline, so we could just de-opt on non-literal values. Or maybe even use code paths as if it's not inline, it's always in the same file. Could be a follow up tweak. Just want your opinion.

@jfmengels
Copy link
Contributor Author

jfmengels commented May 2, 2016

Yeah, I was thinking the same thing, and also checking that n in t.plan(n) is a integer. Wanted to wait before I started working on a shared analysis tool, but this can be an easy tweak for the simple cases, so as you wish.

If we need to play with code paths, I'd rather put it as a follow-up tweak, still can't figure my head around it ^^'

@jfmengels
Copy link
Contributor Author

Updated

t.true(array.indexOf(value) !== -1);
});

/* eslint ava/assertion-arguments: ["error", "never"] */
Copy link
Member

Choose a reason for hiding this comment

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

These comments needs to be updated with the new format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh... yes.

@jfmengels
Copy link
Contributor Author

Updated

@sindresorhus sindresorhus merged commit 1b1986c into master May 4, 2016
@sindresorhus sindresorhus deleted the assertion-arguments branch May 4, 2016 10:12
@sindresorhus
Copy link
Member

Splendid work as always @jfmengels 🙌

leo-cat

@jfmengels
Copy link
Contributor Author

Hey, a DiCaprio gif, it's been a while :D

@sindresorhus
Copy link
Member

I recently stocked up on more of those.

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

Successfully merging this pull request may close these issues.

Rule to enforce the correct number of arguments for assertions
2 participants