Fixes 102

[RebeccaStevens]

Fixes #102

[codecov]

Codecov Report

Merging #110 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #110   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines         338    350   +12     
  Branches      137    146    +9     
=====================================
+ Hits          338    350   +12

Impacted Files	Coverage Δ
src/noArrayMutationRule.ts	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04eed5d...5b5e116. Read the comment docs.

[RebeccaStevens]

We may want to rename ignore-mutation-following-accessor as accessor methods are no longer the only methods checked against. Iteration methods and constructors are also checked against.

[jonaskello]

Do you have a new name for the option in mind?

I guess the best for the users would be if we could keep the old name too for backwards compat.

[RebeccaStevens]

I'm not sure what the best name would be. Maybe something like allow-mutation-when-safe or allow-mutation-on-new.

[jonaskello]

As I understand it the option would allow any array method that does not mutate the array? If that is true, perhaps something like allow-immutable-methods? Perhaps this could be the default behaviour, or is there a scenario where we would want to ban the immutable methods of array?

Please note that I haven't had the opportunity to use this rule a lot myself yet (my current work project only have read-only-array rule), so I may not be the best judge of how to handle this :-).

[RebeccaStevens]

All immutable methods on arrays are allowed by this rule without this option enabled. This option allows mutable methods to be used on arrays as well but only if it is a new array where it is guaranteed that no side-effects will occur as the larger operation is effectively immutable.

Example:

// Allow the mutable method `fill` to be used directly on a new array.
const foo = new Array(10).fill("hello world");

As to whether this should be the default behaviour, I'm not sure. We might want community feedback on this before doing so.

[jonaskello]

Ah, so I misunderstood the purpose of the option, thanks for explaining :-). So the option would allow you to chain a mutation if the previous array method returned a copy. In that case I don't think it should be the default behaviour. IMO all mutation should be disallowed by default.

To convey what the option does with a concise name is seems quite hard. Perhaps ignore-chained-mutation-on-new-array or just ignore-chained-mutation. This might make sense if the rule only works for method chaining where the complete expression returns an array. But perhaps it does even more advanced analysis?

[RebeccaStevens]

No, that is correct; this option only works on chained methods.
I like the name ignore-chained-mutation-on-new-array but it is quite long.

[RebeccaStevens]

I've changed the name of this rule option to ignore-chained-mutation-on-new-array and deprecated the original. Everything should be ready for merging now.

If you want to go with another name, just let me know; it's a really quick fix.

[RebeccaStevens]

I guess a possible shorter name would be to omit a few words. Maybe something more like ignore-mutation-on-new

[jonaskello]

Well, naming is always hard :-). I agree ignore-chained-mutation-on-new-array is too long, and to make it shorter we would have to omit something, making some parts implicit.

The shortest I can think of is ignore-chained. This makes two things implicit:

• It only works following calls that create a new array. However "chained" communicates that you have to make a first call, otherwise you cannot chain something. So the implicit part is that the first call has to return a new array.

• The thing we are ignoring is a mutation. I think this can be OK as the rule prevents mutation, and ignore communicates that the rule is ignored and thus mutation is ignored.

[RebeccaStevens]

ignore-chained has the issue that it isn't clear that this can only be done one new arrays. Someone that doesn't know what this options does may assume that it allows all arrays to be mutated with a chained call.

We could go with ignore-new or ignore-new-array. This clearly states that the option only effects new arrays. Here the chained bit is implicit but the user would still get a good idea of what the option does just based on the name.

[jonaskello]

Yes that is a valid point. I think ignore-new-array sounds good, lets go with that :-).

[RebeccaStevens]

Alright will do. I'll commit the change next time I'm at my computer, should be in an hour or two.

Should it be ignore-new-array or ignore-new-arrays?

[jonaskello]

Is the old rule name still available as an alias? Otherwise, if we should follow semantic versioning strictly I guess the next release would be 6.0.0 (since we are breaking users with the old rule name).

[RebeccaStevens]

The old name is still available as an alias so no need for a major release.

[jonaskello]

Nice 👍 I guess this is ready for merge and release then?

[RebeccaStevens]

Seems to be :)

[jonaskello]

Released in 5.1.0.