No hidden units

[sparksp]

Forbid using _ in place of () - it's valuable to know when a pattern is meant to be a unit and it could be hiding the fact that you're missing data when it's not a unit (perhaps you refactored away from the unit).

One place this comes up is in tests:

-- Fail: `_` used where `()` would be better
test1add1 =
    test "1 + 1 = 2" <|
        \_ ->
            1 + 1 |> Expect.toBe(2)

-- Pass: () is used where () should be
test1add1 =
    test "1 + 1" <|
        \() ->
            1 + 1 |> Expect.equal(2)

-- Note: when refactoring the above test(s) we might forget to use the fuzz value if it's `_`
testAdd1 =
    fuzz int "1 + 1" <|
        \_ ->
            1 + 1 |> Expect.equal(2)

Another might be unused flags passed to an init function:

-- Fail: `_` used where `()` would be better
init : () -> ( Model, Cmd Msg )
init _ =
    ( emptyModel, Cmd.none )

-- Pass: () used where () should be
init : () -> ( Model, Cmd Msg )
init () =
    ( emptyModel, Cmd.none )

-- Note: when adding Flags you might forget to actually use the flags
init : Flags -> ( Model, Cmd Msg )
init flags =
    ( modelFromFlags flags, Cmd.none )

When (not) to enable this rule:

• If you don't care about hiding units.
• This rule is a nitpick - your team might find this rule annoying and it may slow down elm-review on large code bases for little value.

I am looking for:

• Feedback - would you use this rule?
• To know in which package(s) this would fit best
• How to find out where units should be in lambda functions (like the tests) (programmatically)

[MartinSStewart]

• I'd use it
• There are a handful of rules that are for very minor things. Maybe we could combine all of these into a review-nitpicks package
• Not sure if there's an easy way to do this. This rule might be restricted to only function definitions that have type signatures.

[jfmengels]

Would this conflict with NoUnused.Parameters/NoUnused.Patterns? Do those rules already remove units?

How to find out where units should be in lambda functions (like the tests) (programmatically)

I think that if you want to find every case, you need type inference.
A simpler version would just find calls to functions where we have the type annotations

foo : ( () -> something) -> something
foo = something

bar = foo (\_ -> 1) -- We would report it, because we know that the first argument of `foo` is a function whose first argument is ()

Also this one would be easy to report if there is a type annotation:

foo : () -> something
foo _ = -- Report this one here
  something

would you use this rule?

I might, but I'm leaning towards not.

I feel like this adds little value. In most cases, when the argument is (), you already know it. For instance, if you see a test, you know the argument is value-less.

I feel like being pedantic about this is not necessarily worth it, since the "brought value" divided by "annoyance it can give coworkers" ratio is quite low. I imagine that having an automatic fix would help out here.

Performance is more and more an issue for me also, as I am working on a big code-base (~160k LoC), where every additional rules adds maybe like 100ms/200ms (for simple ones. Very rough estimates), so it's unwise to multiply the rules, especially when they bring little value.

To know in which package(s) this would fit best

As @MartinSStewart said, it's a nitpick. I agree that these could be combined into a review-nitpick package. Or maybe everyone has their own review-nitpicks package (for rules that are not available in other nitpick package, no need to duplicate rules), since I doubt someone would want to maintain nitpicks that others propose but that they don't agree with (and IMO righteously so).

[sparksp]

Would this conflict with NoUnused.Parameters/NoUnused.Patterns? Do those rules already remove units?

I don't believe that this would conflict with either of those, they're looking for names that are unused, and consolidating some useless patterns, but unit wasn't one of them.

Performance is more and more an issue for me also, as I am working on a big code-base (~160k LoC), where every additional rules adds maybe like 100ms/200ms (for simple ones. Very rough estimates), so it's unwise to multiply the rules, especially when they bring little value.

That's a very fair point - I'm not working with anything like that scale, most of my projects are relatively small, so at the moment this would give me some value, but I can see where that trade-off might come in to play (I'll add it to the list of when not to use this).

[MartinSStewart]

Performance is more and more an issue for me also, as I am working on a big code-base (~160k LoC), where every additional rules adds maybe like 100ms/200ms (for simple ones. Very rough estimates), so it's unwise to multiply the rules, especially when they bring little value

I'm guessing this is caused by overhead for each rule? Could the logic for lots of nitpick rules be merged together into a single rule to improve performance?

[sparksp]

I'm guessing this is caused by overhead for each rule? Could the logic for lots of nitpick rules be merged together into a single rule to improve performance?

I've been pondering this for a little while now - there may be some nitpick rules that could be merged together, but I think you'd probably lose readability of the rule as a trade-off.

[jfmengels]

I'm guessing this is caused by overhead for each rule?

Probably. It's hard for me to tell at this point.

I think that it's better to leave performance to the tool, and to keep each rule look at its own thing.
We'll find ways to improve the performance of the tool 😉
If you do think of ways, let's discuss that on Slack or in the tool itself, and let's keep this issue to the rule itself ☺️