-
-
Notifications
You must be signed in to change notification settings - Fork 442
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
feat(lint): add ignoreNull
option to noDoubleEquals
rule
#3702
feat(lint): add ignoreNull
option to noDoubleEquals
rule
#3702
Conversation
Biome's `noDoubleEquals` rule initially made an exception when checking against `null`. This not something that is always wanted by linter users, some prefering no exception to this rule in the name of explicitness. The original `eslint` rule, `eqeqeq` has an `always` option to disable specific handling of `null`. Here, I propose an `ignoreNull` option instead, by default set to `true` to not break API (and as it may be your recommended setting), because I find it much more explicit on what it does than the more vague `always`.
CodSpeed Performance ReportMerging #3702 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great contribution, thank you!
As for the etiquette, filing an issue in advance, isn't required, but it's recommended to avoid filing PRs the maintainers may disagree with.
crates/biome_js_analyze/src/lint/suspicious/no_double_equals.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/suspicious/no_double_equals.rs
Outdated
Show resolved
Hide resolved
Thanks! |
Thanks for this feature. It helps a lot when trying to migrate to biome. One thing is there are also some project that standardizes on using |
Note that by default Biome already ignores We could also add a new rule that ensure using consistently |
Note: this is my first contribution/proposal to biome, and I was unsure of if I had to open an issue/discussion first for a relatively simple proposal like this one, or if writing it directly to not waste time was OK. Do not hesitate to tell me the preferred etiquette for potential future contributions, thanks.
Summary
The
noDoubleEquals
rule made an exception when checking againstnull
, as it's a frequently used pattern to check against bothnull
andundefined
.This not something that is always wanted by developers, some preferring no exception to this rule in the name of explicitness.
I also often seen
== null
used as a shortcut when people were unsure if some nullable value could also beundefined
due to other factors not catched by a type checker (out-of-bounds array indexing orRecord
lookup on inexistent properties for example), in which case I do also prefer making such shortcuts less tempting to allow simpler future code updates.The original
eslint
rule,eqeqeq
, has analways
option to disable specific handling ofnull
. Here, I propose anignoreNull
option instead, by default set totrue
to not break API (and as it may be your recommended setting), because I found it much more explicit on what it does than the more vaguealways
, though I don't really have any attachment to that naming if you prefer another one.For implementation and documentation, I tried to look at how rules like
useNamingConvention
declared options and try to rely on more or less the same syntax and macros for its struct.Test Plan
I added tests for invalid cases under the
ignoreNull: false
case, that I namedinvalidNoNull
in the corresponding directory. I checked that their output seemed right.