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
20 stars 9 forks source link

Enable access to the operator Range #121

Closed lue-bird closed 1 year ago

lue-bird commented 1 year ago

elm-syntax only gives us the operator symbol String but having access to the operator Range as well is valuable:

It seems this will be part of elm-syntax v8 https://github.com/stil4m/elm-syntax/issues/74 but now we can patch things up in the meantime :white_flower:

If the PR's solution seems too unstable and you want to wait for v8, I'm happy to wait as well.

Now what's included in this PR?

That means that if this PR would be merged, we still need to change the other error ranges to the operator ranges.

jfmengels commented 1 year ago

I strongly wish this will be part of elm-syntax v8 but now we can patch things up in the meantime (should I add an issue in elm-syntax?)

Yes, same here. It will happen one day :wink:


I'm a bit surprised about this PR, because I just merged the same thing in https://github.com/jfmengels/elm-review-simplify/pull/116/files#diff-6239bccafe2b75c7e560542732acf25d142d08608e1e0f0ffe9b53c09cfb1e0eR6540-R6545

I think we're doing the same thing, except that you're also handling comments, which is a good idea. Can you confirm that we're doing the same thing, or am I misunderstanding something? If we are, then can you try replacing my code by your version? Let's go with your version since it's going to be more correct.

lue-bird commented 1 year ago

Yeah, same thing, just more complicated. Very cool that we accidentally did it at almost the same time. Btw String.split "\n" --> String.lines would be a nice one to add ;)

lue-bird commented 1 year ago

try replacing my code by your version

Ok, I think we're good to go. Merge this anytime

lue-bird commented 1 year ago

merge whenever you feel like it (just avoid a merge commit please :pray:).

before I mess something up, I assume you want "rebase and merge"? image

jfmengels commented 1 year ago

Yes (You do have some failing tests now though). But when you prefer, feel free to use "Squash and merge", your choice.

lue-bird commented 1 year ago

You do have some failing tests now though

Not sure I follow, the github action and the local setup say the tests pass

jfmengels commented 1 year ago

Huh weird, it showed as failing to me yesterday. Alright go ahead then 😊

lue-bird commented 1 year ago

Fixed a bug with pipeline into (parenthesized pipeline into composition). Removing the parens there is incorrect, for example

[[3,4]] |> (negate |> List.map >> List.map)
--> [[-3,-4]]

but

[[3,4]] |> negate |> List.map >> List.map
--> TYPE MISMATCH

This might have been handled by your old rule but now we at least have a test for it