[New] jsx-boolean-value: add inverse option for always/never
#1250
Conversation
ljharb
force-pushed
the
boolean_inverse
branch
3 times, most recently
from
e705c7d to
59398c2
Compare
|
ping for reviews :-) |
tests/lib/rules/jsx-boolean-value.js
Outdated
| @@ -28,13 +28,18 @@ var ruleTester = new RuleTester({parserOptions}); | |||
| ruleTester.run('jsx-boolean-value', rule, { | |||
| valid: [ | |||
| {code: '<App foo />;', options: ['never']}, | |||
| {code: '<App foo />;', options: ['always', {never: ['foo']}]}, | |||
There was a problem hiding this comment.
Should we also cover the always part in these test cases? E.g. checking that bar={true} is still valid when the config is ['always', {never: ['foo']}].
ljharb
force-pushed
the
boolean_inverse
branch
6 times, most recently
from
89b0dbe to
42eb4b1
Compare
|
@yannickcr @lencioni @EvNaverniouk if you have a chance to review this that'd be appreciated :-) |
| var NEVER_MESSAGE = 'Value must be omitted for boolean attributes'; | ||
| var ALWAYS_MESSAGE = 'Value must be set for boolean attributes'; | ||
| const NEVER_MESSAGE = 'Value must be omitted for boolean attributes{{exceptionsMessage}}'; | ||
| const ALWAYS_MESSAGE = 'Value must be set for boolean attributes{{exceptionsMessage}}'; |
There was a problem hiding this comment.
It might be nice to move some of this into a helper function that takes exceptions as an argument, so this code only has to run in the event of a warning.
lib/rules/jsx-boolean-value.js
Outdated
| const isException = exceptions.has(propName); | ||
|
|
||
| const isAlways = configuration === ALWAYS ? !isException : isException; | ||
| const isNever = configuration === NEVER ? !isException : isException; |
There was a problem hiding this comment.
I find these two ternaries less clear than if/else. I had to work through the logic here, which slowed me down. It might be worth converting.
| The following patterns are considered warnings when configured `"never"`: | ||
| The second argument is optional: if provided, it must be an object with a `"never"` property (if the first argument is `"always"`), or an `"always"` property (if the first argument is `"never"`). This property’s value must be an array of strings representing prop names. | ||
|
|
||
| The following patterns are considered warnings when configured `"never"`, or with `"always", { "never": ["personal"] }`: |
There was a problem hiding this comment.
I'm on the fence about whether this should be never/always, or something more general like exceptions or except. As it is written here, it is possible to have both never and always which would be a little weird. I think it is fine, but I think a unified option might make the documentation a little clearer and the code a small amount simpler. What do you think?
There was a problem hiding this comment.
To me, "exceptions" means "these things are not checked" - the semantic here is, everything is forced to something: some things are forced to always, and some to never.
There was a problem hiding this comment.
Ah I see. That makes sense.
|
@lencioni updated per your comments; i'll merge tomorrow if there's no other issues. |
Fixes #1249