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

Reassembling all parts of a whole can be replaced by the original whole #294

Open lue-bird opened 4 months ago

lue-bird commented 4 months ago
( Tuple.first originalTuple, Tuple.second originalTuple )
--> originalTuple

Tuple.pair (Tuple.first originalTuple) (Tuple.second originalTuple)
--> originalTuple

(found in the FSharpLint default config)

If type inference becomes available in the future, the same can be done for non-extensible records, although this might be a bit more controversial because same field names does not necessarily imply same semantics.

Somewhat related: https://github.com/jfmengels/elm-review-simplify/issues/29

jfmengels commented 4 months ago

Sounds good to me 👍

lue-bird commented 3 months ago

I don't think I like this anymore, for the same reason I wouldn't like the same rule for records: same position does not necessarily imply same semantics.

Feel free to reopen if you disagree.

jfmengels commented 3 months ago

Same position does not necessarily imply same semantics.

I'm not sure I follow. I agree with that sentence, but with the code to be simplified above, you don't get to add new semantics (like names).

I can see the code below as giving names to an existing thing as having some potential value.

let
    ( buyerId, products ) =
        userAndBasket
in
( buyerId, products )

but I can't see what you'd gain by doing

( Tuple.first userAndBasket, Tuple.second userAndBasket )
--> instead of
userAndBasket

Same for Tuple.pair

(I'll re-open for now)

lue-bird commented 3 months ago

for example

pinnedSongAndPinnedAlbum : ( Song, Album )
in
List.NonEmpty.cons
    ( pinnedSongAndPinnedAlbum |> Tuple.first
    , pinnedSongAndPinnedAlbum |> Tuple.second
    )
    |> artistIdUi

the fact that the album is represented as a list is a pure coincidence and could change anytime, which would mean a lot more work refactoring compared to now.

Other than these (arguably quite rare) situations, the fix will have to be quite careful to limit itself, like not applying a fix if there are comments

( Tuple.first userAndBasket -- the old basket
, Tuple.second userAndBasket
)

or other kinds of functions that other checks currently consider equivalent

( userAndBasket |> (\( theOldBasket, _ ) -> theOldBasket)
, Tuple.second userAndBasket
)

Overall, I couldn't be mad if this check was introduced, I just don't want users getting annoyed about "an overly smart" check that made future maintenance a bit harder.