Add elm-review rule Simplify
Overview
Add elm-review rule Simplify
This one is fairly in depth, and will attempt to simplify many different expressions.
Expressions will be resolved to their simplest forms
x == x -> True
if _ then x else x -> x
There are also many optimizations like Cmd.batch [ a ] -> a,
removal of explicit Cmd.none in batches, and more.
See https://package.elm-lang.org/packages/jfmengels/elm-review-simplify/2.1.3/Simplify/ for all the simplifications.
For easier grokking, I applied every category of fix in an individual commit
(This first one is important
Simplify: immediately evaluated anonymous function
This one is a special case, instead of the suggested fixes I was able to just update the definition of projectSetServerLoading to swap the ServerUuid and Project arguments, allowing easy pipelining and keeping with the common code style of Exosphere.
The other easy fix in this case could have been
GetterSetters.projectSetServerLoading project serverUuid
|> GetterSetters.modelUpdateProject model-- ELM-REVIEW ERROR ------------------------- src/Rest/ApiModelHelpers.elm:65:21
Simplify: Anonymous function is immediately invoked
64| ( project
65| |> (\p -> GetterSetters.projectSetServerLoading p serverUuid)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
66| |> GetterSetters.modelUpdateProject model
This expression defines a function which then gets called directly afterwards,
which overly complexifies the intended computation.
While there are reasonable uses for this in languages like JavaScript, the same
benefits aren't there in Elm because of not allowing name shadowing.
Here are a few ways you can simplify this:
- Remove the lambda and reference the arguments directly instead of giving them
new names
- Remove the lambda and use let variables to give names to the current arguments
- Extract the lambda to a named function (at the top-level or defined in a let
expression)
=========================================== src/Rest/ApiModelHelpers.elm:104:21
Simplify: Anonymous function is immediately invoked
103| ( project
104| |> (\p -> GetterSetters.projectSetServerEventsLoading p serverUuid)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
105| |> GetterSetters.modelUpdateProject model
This expression defines a function which then gets called directly afterwards,
which overly complexifies the intended computation.
While there are reasonable uses for this in languages like JavaScript, the same
benefits aren't there in Elm because of not allowing name shadowing.
Here are a few ways you can simplify this:
- Remove the lambda and reference the arguments directly instead of giving them
new names
- Remove the lambda and use let variables to give names to the current arguments
- Extract the lambda to a named function (at the top-level or defined in a let
expression)Simplify: Use `String.concat` instead
-- ELM-REVIEW ERROR ---------------------- src/Style/Widgets/DataList.elm:693:28
(fix) Simplify: Use String.concat instead
692| filters
693| |> String.join ""
^^^^^^^^^^^
694| |> Murmur3.hashString 4321
Using String.join with an empty separator is the same as using String.concat.Simplify: Use <| instead of <<, |> instead of >>
-- ELM-REVIEW ERROR -------------------- src/Style/Widgets/MultiMeter.elm:108:25
Simplify: Use <| instead of <<
107| , Element.htmlAttribute (HtmlA.id tooltipLink)
108| , (position << Element.map never) <|
^^
109| Element.el [ Element.htmlAttribute (HtmlA.style "pointerEvents" "none") ]
Because of the precedence of operators, using << at this location is the same as
using <|.
Please use <| instead as that is more idiomatic in Elm and generally easier to
read.
I think I can fix this. Here is my proposal:
107| , Element.htmlAttribute (HtmlA.id tooltipLink)
108| , (position << Element.map never) <|
109| , position <| Element.map never <|
109| Element.el [ Element.htmlAttribute (HtmlA.style "pointerEvents" "none") ]Simplify: Cmd.batch [ a ] -> a
-- ELM-REVIEW ERROR ----------------------------- src/State/ViewState.elm:424:39
Simplify: Unnecessary Cmd.batch
423| ( GetterSetters.modelUpdateProject sharedModel project
424| , Cmd.batch [ Ports.instantiateClipboardJs () ]
^^^^^^^^^
425| )
Cmd.batch with a single element is equal to that element.
I think I can fix this. Here is my proposal:
423| ( GetterSetters.modelUpdateProject sharedModel project
424| , Cmd.batch [ Ports.instantiateClipboardJs () ]
425| , (Ports.instantiateClipboardJs ())
425| )Simplify: Don't use case to destructure tuples
-- ELM-REVIEW ERROR --------------------------------- src/State/State.elm:100:17
Simplify: Use a let expression to destructure data
99| case updateValid msg model of
100| ( newModel, nextMsg ) ->
^^^^^^^^^^^^^^^^^^^^^
101| ( Ok newModel, nextMsg )
It is more idiomatic in Elm to use a let expression to define a new variable
rather than to use pattern matching. This will also make the code less indented,
therefore easier to read.
I think I can fix this. Here is my proposal:
98| Ok model ->
99| case updateValid msg model of
100| ( newModel, nextMsg ) ->
101| let ( newModel, nextMsg ) = updateValid msg model
101| in
101| ( Ok newModel, nextMsg )Simplify: List.member
This replaces uses of List.any that do a simple comparison with List.member
-- ELM-REVIEW ERROR ----------------------- src/Helpers/GetterSetters.elm:254:12
Simplify: Use List.member instead
348| server.osProps.details.volumesAttached
349| |> List.any (\v -> v == volumeUuid)
^^^^^^^^
This call to List.any checks for the presence of a value, which what List.member
is for.
I think I can fix this. Here is my proposal:
349| |> List.member volumeUuidSimplify: List.map and List.concat can be combined using List.concatMap
-- ELM-REVIEW ERROR ------------------------------- src/Helpers/String.elm:21:16
Simplify: List.map and List.concat can be combined using List.concatMap
20| |> List.map (String.split "-")
21| |> List.concat
^^^^^^^^^^^
22| |> List.map (String.split ".")
List.concatMap is meant for this exact purpose and will also be faster.
I think I can fix this. Here is my proposal:
19| |> String.split " "
20| |> List.map (String.split "-")
21| |> List.concat
22| |> List.concatMap (String.split "-")
22| |> List.map (String.split ".")
√ Do you wish to apply this fix? ... yesSimplify: List.map |> List.filter -> List.filterMap
Simplify: List.map and List.filterMap identity can be combined using
List.filterMap
49| |> List.map Result.toMaybe
50| |> List.filterMap identity
^^^^^^^^^^^^^^^^^^^^^^^
51| |> List.head
List.filterMap is meant for this exact purpose and will also be faster.
I think I can fix this. Here is my proposal:
48| |> List.map (\line -> Parser.run varParser line)
49| |> List.map Result.toMaybe
50| |> List.filterMap identity
51| |> List.filterMap Result.toMaybe
51| |> List.head
√ Do you wish to apply this fix? ... yesSimplify: Consecutive literal lists should be merged
Simplify: Consecutive literal lists should be merged
67| String.join " " <|
68| List.concat
^^^^^^^^^^^
69| [ [ usedCount ]
Try moving all the elements from consecutive list literals so that they form a
single list.
I think I can fix this. Here is my proposal:
74| [ usedLabel ]
75| , [ "of" ]
76| , [ limitCount ]
77| , [ limitLabel ]
78| , [ "of" , limitCount , limitLabel ]
78| ]
√ Do you wish to apply this fix? ... yesSimplify: Set.fromList with a single element can be replaced using Set.singleton
Simplify: Set.fromList with a single element can be replaced using Set.singleton
481| , filterTypeAndDefaultValue =
482| DataList.MultiselectOption <| Set.fromList [ "me" ]
^^^^^^^^^^^^
483| , onFilter =
You can replace this call by Set.singleton with the list element itself.
I think I can fix this. Here is my proposal:
481| , filterTypeAndDefaultValue =
482| DataList.MultiselectOption <| Set.fromList [ "me" ]
483| DataList.MultiselectOption <| Set.singleton "me"
483| , onFilter =
√ Do you wish to apply this fix? ... yesSimplify: Use Maybe.map instead
Simplify: Use Maybe.map instead
51| | maybeSupportableResource =
52| Maybe.andThen (\itemType -> Just ( itemType, Nothing )) maybeItemType
^^^^^^^^^^^^^
53| }
Using Maybe.andThen with a function that always returns Just is the same thing
as using Maybe.map.
I think I can fix this. Here is my proposal:
51| | maybeSupportableResource =
52| Maybe.andThen (\itemType -> Just ( itemType, Nothing )) maybeItemType
53| Maybe.map (\itemType -> ( itemType, Nothing )) maybeItemType
53| }
√ Do you wish to apply this fix? ... yesSimplify: Unnecessary comparison with boolean
Simplify: Unnecessary comparison with boolean
511| |> List.filter (\n -> n.status == "ACTIVE")
512| |> List.filter (\n -> n.adminStateUp == True)
^^^^^^^
513| |> List.filter (\n -> n.isExternal == False)
The result of the expression will be the same with or without the comparison.
I think I can fix this. Here is my proposal:
511| |> List.filter (\n -> n.status == "ACTIVE")
512| |> List.filter (\n -> n.adminStateUp == True)
513| |> List.filter (\n -> n.adminStateUp)
513| |> List.filter (\n -> n.isExternal == False)
√ Do you wish to apply this fix? ... yesSimplify: The values in both branches is the same.
Simplify: The values in both branches is the same.
54| NotLaunchedWithCustomWorkflow ->
55| if exoOriginProps.exoServerVersion < 3 then
^^
56| ITypes.Hidden
The expression can be replaced by the contents of either branch.
I think I can fix this. Here is my proposal:
54| NotLaunchedWithCustomWorkflow ->
55| if exoOriginProps.exoServerVersion < 3 then
56| ITypes.Hidden
57| ITypes.Hidden
57|
58| else
59| -- Deployed without a workflow
60| ITypes.Hidden
61|
62| LaunchedWithCustomWorkflow customWorkflow ->
√ Do you wish to apply this fix? ... yesSimplify: `not` is used on a negatable boolean operation
Simplify: `not` is used on a negatable boolean operation
357| List.filter
358| (\server -> not (server.id == serverId))
^^^
359| model.servers
You can remove the `not` call and use `/=` instead.
I think I can fix this. Here is my proposal:
357| List.filter
358| (\server -> not (server.id == serverId))
359| (\server -> (server.id /= serverId))
359| model.servers
√ Do you wish to apply this fix? ... yesI found no more errors!
Discussion
I love this rule. It taught me several functions I did not know about (Set.singleton, List.member) and makes it really easy to just write quickly and auto-optimize/fix long chains (e.g. List.map |> List.filter)
With auto-fixes, it makes cleanup easy