elm-review icon indicating copy to clipboard operation
elm-review copied to clipboard

Decouple error from fix location

Open lue-bird opened this issue 2 years ago • 2 comments

The range the error is shown for can already be different from where fixes are being made.

There are also rules where the use of certain functions/imports/declarations/names/... in one module should trigger fixes in another module. Example: using a name from Nat.N<x> will generate a type alias declaration for <x> in Nat. NoMissingRecordFieldLens already does this, but needs to put the error location on the generation module header name, which leaves users wondering why it was created while showing a rather unhelpful error range. A similar example is generating tests from examples which I've always wanted to implement nicely.

I suggest adding

Review.Fix.inModule : Review.Rule.ModuleKey -> Fix -> Fix

used like

[ -- defaults to error location module
  Review.Fix.removeRange importRange
, Review.Fix.inModule originModule.key
    (Review.Fix.removeRange usedDeclarationRange)
]

lue-bird avatar May 16 '22 06:05 lue-bird

I think this would be nice to have, as that could make some rules way more powerful.

I think we'd need to think of how to report this to the user in a terminal, in an editor (I'm not sure that multi-file fixes would be compatible with LSP), and how to communicate that to any programs that use the JSON output of the CLI (would probably be a breaking change).

The API for Review.Rule.errorForElmJsonWithFix would not be compatible with the idea, but we could add a new function for that. For instance, it could take an additional argument like List ( ModuleKey, List Fix ). But in that case [ ( moduleKey, Review.Fix.inModule otherModuleKey (Review.Fix.removeRange usedDeclarationRange ) ] would be problematic :thinking:

jfmengels avatar May 17 '22 20:05 jfmengels

Some more examples of where this would be useful:

  • a rule that reports function declarations where arguments are ignored. The fix – removing that argument from all usages and the declaration – is only possible in one go, with fixes in multiple modules
  • the same rule for unused arguments in type declarations (aka for the forbid phantom types rule)
  • renaming a declared thing in all usages and the declaration, e.g. converting it to camel case or removing unnecessary _-suffixes

lue-bird avatar Feb 22 '24 19:02 lue-bird