jfmengels / elm-review-simplify

Provides elm-review rules to simplify your Elm code
https://package.elm-lang.org/packages/jfmengels/elm-review-simplify/latest/
BSD 3-Clause "New" or "Revised" License
19 stars 9 forks source link

The fix removed a useful comment #46

Closed lydell closed 1 year ago

lydell commented 2 years ago

The rule simplifed some list code:

                 (DesignSystem.fontColor Blue500
-                    :: Element.Font.underline
-                    -- This class is used in app.ts.
-                    :: Element.htmlAttribute (Html.Attributes.class "js-ReloadPage")
-                    :: [ DesignSystem.fontWeight DesignSystem.Bold ]
+                    :: [ Element.Font.underline, Element.htmlAttribute (Html.Attributes.class "js-ReloadPage"), DesignSystem.fontWeight DesignSystem.Bold ]
                 )

But I lost that -- This class is used in app.ts. comment.

Thoughts?

jfmengels commented 2 years ago

Yeah, this is unfortunate, but it's really hard to handle comments at the moment, and I don't know of a design that would make comments work out better.

I see 3 ways of improving this:

  1. Have elm-syntax better attach the comments to the AST. Unfortunately I have no clue how this could be achieved, or what "better attached" would mean in practice.
  2. Have elm-review report a warning whenever it notices that comments have changed after a fix, and show that in the fix prompt.
  3. In this specific instance, be a bit more conservative on the fix. I thin we're basically replacing the from Blue500 to Element.Font.underline by [, and we could try to only replace ::, which may help with this case. But elm-syntax doesn't currently provide the range information about the operator, hence why this removes more than necessary.

In the meantime, this is what the fix prompt is for, so that you can re-add the comment or apply the fix manually yourself. So I'm happy that you were able to figure this out (at some point in the prompt or in the git diff). I would love to solve it, but even the easiest solution would require a major version of elm-syntax (which we will focus on one day).