Remove boolean -> nil mutations
#1018
Conversation
|
@dgollahon I like the rationale. I'm onboard. Interestingly circleCI sneaked in here again, I'll disable it tomorrow and get this merged. |
|
Sounds great, thanks! Should I add a changelog entry? I don't see an "Unreleased" section but I can add one if you'd like. |
Feel free to do so. I'm very curious on why CI is not green at this point, will debug this in around 4h, once I've the kids in bed. |
Removes the following mutations:
- `true` -> `nil`
- `false` -> `nil`
Rationale:
- Since `true` is already mutated to `false` and vice versa, we already have a truthiness change and `nil` does not provide anything else in that direction. It is very unlikely to get a mutation from mutating to `nil` that we do not get from inverting the boolean.
- `nil` is actually the less primitive construct and easier to misuse.
- nil.methods - false.methods # => [:to_a, :to_i, :to_f, :to_h, :to_r, :rationalize, :to_c]
This allows for more cases where a `nil` is somehow accidentally turned into an empty hash, array, 0 value, etc.
- false.methods - nil.methods # => []
- false.methods == true.methods # => true
- It forces `nil` to be present in cases that are extremely hard to assert (inside conditionals that do not return the boolean expression) or, even if they are possible to assert, it is often awkward to do so. As a motivating example I like to use hashes as an asserted case statement (ex: `if { 'confirmed' => true, 'denied' => false}.fetch(enum_value) ...` but `mutant` would force me to make the value of the `'denied'` key to `nil` which is not an improvement in semantics or readability and very difficult to assert if the result of that expression is never part of a public API.
- It also results in fewer mutations being generated which is beneficial for CI runs and faster local feedback loops.
- This change also has the slight side benefit of removing more code from `mutant` than it adds.
|
Updated the changelog. |
|
To put a bit of a more axiomatic reasoning behind this change:
A mutation of the type signature from
The exusting mutaiton from |
|
@dgollahon merged in repush as #1019 |
Removes the following mutations:
true->nilfalse->nilRationale:
trueis already mutated tofalseand vice versa, we already have a truthiness change andnildoes not provide anything else in that direction. It is very unlikely to get a mutation from mutating tonilthat we do not get from inverting the boolean.nilis actually the less primitive construct and easier to misuse.This allows for more cases where a
nilis somehow accidentally turned into an empty hash, array, 0 value, etc.nilto be present in cases that are extremely hard to assert (inside conditionals that do not return the boolean expression) or, even if they are possible to assert, it is often awkward to do so. As a motivating example I like to use hashes as an asserted case statement (ex:if { 'confirmed' => true, 'denied' => false }.fetch(enum_value) ...butmutantwould force me to make the value of the'denied'key tonilwhich is not an improvement in semantics or readability and very difficult to assert if the result of that expression is never part of a public API.mutantthan it adds.@mbj Let me know what you think. I figured it was probably easier to just go ahead an open a PR for discussion rather than create an issue since it was a fairly easy change to make.