lue-bird/elm-review-pattern/1.0.2

NOTE: the latest version is 1.0.3

Review.Pattern.Record

Forbid { field, ... } record patterns.

forbid : Rule

Forbid { field, ... } record patterns.

config =
    [ Review.Pattern.Record.forbid
    ]

For example

a ({ field } as record) =
    f field record

will be fixed to

a record =
    f record.field record

Patterns without as will not get an auto-fix, so either manually apply this change or add a temporary as.

why

  • Readability and error prone-ness: Similar to exposing from an import, it becomes hard to follow where a certain value originated from

    checkForConcatOnMap { nodeRange, firstArg } =
        case toSpecificCall ( "List", "map" ) firstArg of
            Just { fnRange } ->
                let ...
                in
                Fix.replaceByExpression nodeRange firstArg
                    ++ Fix.replace fnRange "List.concatMap"
    
    
            Nothing ->
                Nothing

    Since fnRange, nodeRange and firstArg could have all come from either call, it's really hard to trace their origin without jumping around in your code.

    checkForConcatOnMap concatCall =
        case toSpecificCall ( "List", "map" ) concatCall.firstArg of
            Just mapCall ->
                let ...
                in
                Fix.replaceByExpression concatCall.nodeRange concatCall.firstArg
                    ++ Fix.replace mapCall.fnRange "List.concatMap"
    
    
            Nothing ->
                Nothing

    Here it's always very clear what thing a field refers to. Don't you agree this is much easier to understand?

  • less syntax to learn to use right and keep in your head

  • Name clashes: Using record destructuring relies on the assumption that no values with the same name will ever in the lifetime of this file be introduced.

    module Ui exposing (icon)
    
    
    icon { image, style } =
        ...

    future iteration

    module Ui exposing (icon, image)
    
    
    image { proportions, url } =
        ...
    
    
    icon { image, style } =
        ...

    Ok, we have to change this one completely unrelated field into a record access. That's not too bad.

    module Ui exposing (icon, image)
    
    
    image { proportions, url } =
        ...
    
    
    icon ({ style } as config) =
        ...

    future iteration

    module Ui exposing (icon, image, url)
    
    
    image { proportions, url } =
        ...
    
    
    icon ({ style } as config) =
        ...
    
    
    url { path, openInNewTab } =
        ...

    Oh well, another clash

    module Ui exposing (icon, image, url)
    
    
    image ({ proportions } as config) =
        ...
    
    
    icon ({ style } as config) =
        ...
    
    
    url { path, openInNewTab } =
        ...

    This mixed use of record access and record destructuring is quite ugly I'd say. Maybe just use record access consistently so you don't have these problems?

  • Scalability: You can't really use record destructuring anywhere except at the most outer argument/let level because the field names quickly lose context and introduce name clashes.

    checkForConcatOnMap { nodeRange, firstArg } =
        case toSpecificCall ( "List", "map" ) firstArg of
            Just { fnRange, nodeRange } ->
                let ...
                in
                Fix.replaceByExpression nodeRange nodeRange -- well...
                    ++ Fix.replace fnRange "List.concatMap"
    
    
            Nothing ->
                Nothing