Cleanup

[lue-bird]

changes overview

As always, if you dislike how certain changes turned out in the end, say for example removing isEmptyList in favor of a case of on getListLiteral, I'll happily refactor to a consistent style you prefer.

• rework composition
  • introduce a "by later function name" like with functionCallChecks
• always take qualifieds as a tuple, not separate arguments
• prefer replaceBySubExpressionFix over keepOnlyFix to avoid uncaught parenthesis bugs (and I think this will actually catch a few, like when an if is used as the argument in <| but the kept branch is ... |> ...)
• use replaceBySubExpressionFix etc instead of manual fixes to remove complexity and bugs and to avoid unnecessary parens
• always collapse and reduce getSpecificFunctionCall, getSpecificValueOrFunction
• Collection etc get their moduleName : ModuleName instead of moduleName : String
• prefer qualifiedToString instead of raw Strings in error info. This should also make it easier to embed them into future shared error info formats
• refactor away all those if Maybe.withDefault False (Maybe.map ... and case Maybe.andThen ... arg
• lean into firstThatReportsError even more
• correct a bug with Dict.size (Dict.fromList [b, c]) being replaced by 2 and Dict.size (Dict.fromList [(1,1), (1,2)] being replaced by 2 instead of 1
• replace matching on a singleton list literal with getListSingleton to get List.singleton for free
• rework isAlwaysMaybe, isAlwaysResult, resultValues, maybeValues, ... to constructs, sameInAllBranches
  • correct bug with e.g. Result.toMaybe (if x then Err a else Ok b) being fixed to Nothing
  • enable mapError f (if x then Err a else Err b) fix to f (if x then a else b)
  • correct with e.g. Maybe.andThen (always (Just a)) maybe being fixed to maybe

to discuss

• adding

  qualifyToString fullyQualified resources =
    qualifiedToString (qualify fullyQualified resources)`

  • more convenient and readable
  • easier to mix up with qualifiedToString
• changing e.g.

  compositionChecks checkInfo =
    firstThatReportsError
        [ \() -> basicsIdentityCompositionChecks checkInfo
        , \() -> basicsNotCompositionChecks checkInfo
        , \() -> basicsNegateCompositionChecks checkInfo
        , \() -> ...
        ]
        ()

  to

  compositionChecks =
    firstThatReportsError
        [ basicsIdentityCompositionChecks 
        , basicsNotCompositionChecks 
        , basicsNegateCompositionChecks 
        , \checkInfo -> ...
        ]

  • the lower one is IMO more readable and the sub-checks are more atomized
  • performance in the lower one might be a tad worse because of currying
• Switching from returning a list of errors for each check to maybe one error
  • seems more in line with what's actually being reported
  • constructing and deconstructing lists might be more expensive than with Maybe?
  • firstThatReportsError becomes almost findMap, which is a bit more intuitive

follow-up PR

I'd like to to overhaul error infos to common formats (those already marked TODO and many more like adding even more general checks like andThenPureChecks for the types) and polish outliers like "Unnecessary wildcard argument argument". I could add the changes them here but they probably need much more discussion and generally more time which doesn't help merging the current, unrelated changes in this PR.

I can start this PR work right after this one is merged or later depending on if you want to tweak some things after merge.

[jfmengels]

https://github.com/jfmengels/elm-review-simplify/pull/135/commits/9aed1dc48f7ac82cd1013135f918e1b557049c99 contained a bunch of different changes and was a bit hard to review, it would have been nice if these changes were in different commits.

Here's something I like to do: When I'm doing a change, and I then notice a different thing I should (or want to) change first, I git stash my changes so I start again with a clean slate, do my changes, commit, and then git stash pop to resume the original work. Sometimes I stack these changes and that's okay (it's actually useful, because it reminds me where I needed to continue work 😄 ). Hope this helps!

[lue-bird]

When I'm doing a change, and I then notice a different thing I should (or want to) change first, I git stash my changes so I start again with a clean slate, do my changes, commit, and then git stash pop

Oh, that's super helpful!

[lue-bird]

I know I keep adding commits for minor things but I consider this done and review-able/merge-able

[jfmengels]

Thank you for this extremely large and great changes! The commit story was much better in the second half, much more enjoyable to review :relaxed:

I hope you had fun doing these changes!

[lue-bird]

Thanks for reviewing! Did you take a look at the "to discuss" section in the PR summary?

[jfmengels]

Did you take a look at the "to discuss" section in the PR summary?

Oops, forgot to reply to those.

qualifyToString sounds good to me :+1:

Let's do your proposed changes for compositionChecks, I was thinking of that as well

Switching from returning a list of errors for each check to maybe one error

That sounds good to me. I guess there are very few places (if any) where we return multiple errors, right?

I can start this PR work right after this one is merged or later depending on if you want to tweak some things after merge.

Feel free to do it, I don't plan on tweaking the code at the moment.

[jfmengels]

Oh, you mentioned you fixed a few bugs. We should probably mention that in the CHANGELOG.md as well, and ideally write some tests if you didn't already.