-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
New rule: No unused record fields #15
Comments
Fixes #15 Avoids a false positive between imported types and imported custom type constructors.
I haven't made any recent progress. but I have pushed a branch Should someone want to look into this, I guess you can run the rule from that branch on a codebase and see what it's reporting and shouldn't, and start from there (if you want to start from what I did). There seems to be tests that I've skipped but that should be handled. Also, in the long run (in a thought that came after the creation of my branch), I think we should merge this rule with |
The `pressedButtons` field in the `RoundInitialState` type has been unused since 7890b9e ("Refactor and unify input handling"). It was originally added in a9eec80 ("Make R replay the previous round"), under the name `pressedKeys`. I did a `git bisect` with that commit as the last known good one, saying `git bisect bad` if and only if I could remove the field in the type definition without the compiler pointing out any locations where it was read. (It has been written to all along, but that doesn't count as used.) Unfortunately, [there doesn't seem to exist an `elm-review` rule for unused fields][issue]. [issue]: jfmengels/elm-review-unused#15 💡 `git show --color-words=.`
What the rule should do:
Report unused fields in records, records defined in type aliases and in records that are arguments to custom types constructors.
What problems does it solve:
It will help remove and uncover more unused code.
Example of things the rule would report:
In the following example, the
unused
field is not used and should be reported.The advice would be to use
{ a | x : Int }
or to remove theunused
field.In the following example, the
unused
field is not used in any places where we declareRecord
to be usedThe same result should appear if the record was wrapped in a custom type constructor.
Example of things the rule would not report:
Notes:
I think this can be quite a tricky rule to implement, and it will likely require many, many iterations to get rid of all false positives and false negatives. If we have to choose between one, we should choose false negatives, as that will avoid putting unnecessary annoyances on the user (especially since we can't disable rules locally).
I imagine we could potentially have a more aggressive version of the rule, under a different name or with a configuration flag, that is more aggressive. That would allow people to do some more manual removals when they fancy to, and allow us to experiment and improve the rule.
Things start to get tricky when you pass in records to other functions, because you will have to dig more into whether something is used or not.
I am looking for:
NoUnused.Fields
andNoUnused.RecordFields
. I prefer the former, but I wonder if the additional explicitness in the latter makes it any clearer.I think the first step for writing this rule would be handling the first example, and then seeing what false positives come out of that and fix them. We can deal with record aliases and ones in custom types later.
I don't know how things will work when type annotations are missing, so let's skip the annotated values for the initial phase.
The text was updated successfully, but these errors were encountered: