jfmengels / elm-review

Analyzes Elm projects, to help find mistakes before your users find them.
https://package.elm-lang.org/packages/jfmengels/elm-review/latest/
Other
255 stars 13 forks source link

Add ModuleNameLookupTable support for operators #106

Closed SiriusStarr closed 3 years ago

SiriusStarr commented 3 years ago

Per #105.

Note that this PR flips prioritization of importedFunctions to overwrite older names by newer names. This is consistent with the behavior of the compiler, which will allow duplicate operator imports (though the only duplicate operators that exist are (|=) and (|.) in Parser and Parser.Advanced. Compare the error message for https://ellie-app.com/fsQQPffw3BVa1 vs https://ellie-app.com/fsQQJ9tMgf4a1 I would not expect it to cause other issues, as duplicate non-operator imports trigger a compile error when the name is actually used.

Note also that I used Review.Test.Dependencies in the tests. Unsure if this is acceptable, as it looks like the tests instead used an incomplete dependency in Fixtures.Dependencies instead, but I don't know if that is a holdover from before Review.Test.Dependencies existed or something. Obviously the Url/Parser dependencies could be created in Fixtures instead if desired.

jfmengels commented 3 years ago

Awesome, thank you very much!

You forgot to update the documentation for moduleNameAt, which I just did. I also put the mentions of the operator nodes last, as they are less likely to be relevant to the reader.

I added the Hacktoberfest label, in case you want to get a T-shirt (or plant a tree, which I hope they still do).

jfmengels commented 3 years ago

Released in 2.5.3 :+1:

SiriusStarr commented 3 years ago

You forgot to update the documentation for moduleNameAt, which I just did.

I knew I was going to do that... I had it in my head to do and then got sidetracked by the weirdness around the fact that the compiler will let you import ambiguous operators. :rofl: