Follow HLint suggestions?

[philderbeast]

Running hlint-3.8, I'm finding 148 hints. Are you open to following these suggestions? If so then I'd be happy to contribute those changes.

• Avoid lambda, 1 hint
• Eta reduce, 12 hints
• Fuse foldr/map, 1 hint
• Move brackets to avoid $, 6 hints
• Redundant $, 32 hints
• Redundant <$>, 17 hints
• Redundant bracket, 12 hints
• Replace case with fromMaybe, 3 hints
• Use $>, 2 hints
• Use <&>, 1 hint
• Use >=>, 1 hint
• Use asks, 5 hints
• Use camelCase, 2 hints
• Use const, 4 hints
• Use evalState, 2 hints
• Use fold, 1 hint
• Use for, 1 hint
• Use fromMaybe, 1 hint
• Use id, 6 hints
• Use newtype instead of data, 3 hints
• Use record patterns, 4 hints
• Use tuple-section, 2 hints
• Use uncurry, 2 hints
• Use unless, 3 hints
• Use unwords, 1 hint
• Use void, 5 hints
• Use when, 2 hints

[doyougnu]

Yes please! We are thankful for the help. Most of these should be extremely minor changes. The only one that will be difficult and that I think you should avoid are the incomplete pattern matches in the Evaluator. Fixing these will mean re-architecting the Evaluator and is just not worth it at this time.

[gelisam]

As discussed in #234, we do not consider all of hlint's suggestions to be improvements. @doyougnu, @david-christiansen and I met and agreed that the following suggestions are usually good:

[ ] Use $>, 2 hints
[ ] Use <&>, 1 hint
[ ] Use >=>, 1 hint
[ ] Use asks, 5 hints
[ ] Use evalState, 2 hints
[ ] Use fold, 1 hint
[ ] Use for, 1 hint
[ ] Use fromMaybe, 1 hint
[ ] Use id, 6 hints
[ ] Use unless, 3 hints
[ ] Use unwords, 1 hint
[ ] Use void, 5 hints
[ ] Use when, 2 hints

and the following suggestions are usually bad:

[ ] Avoid lambda
[ ] Eta reduce, 12 hints
[ ] Fuse foldr/map, 1 hint
[ ] Move brackets to avoid $, 6 hints
[ ] Redundant $, 32 hints
[ ] Redundant <$>, 17 hints
[ ] Redundant bracket, 12 hints
[ ] Replace case with fromMaybe, 3 hints
[ ] Use tuple-section, 2 hints
[ ] Use uncurry, 2 hints

This leaves the following suggestions which are sometimes good and sometimes bad.

1. Use camelCase, 2 hints: some codebases use camelCase for exported identifiers and snake_case for private identifiers. the 2 hints happen to point at a case where we should have used camelCase, so this would probably be a good hint for our codebase.
2. Use newtype instead of data, 3 hints: newtype might be more space efficient than data, but it does not have the same meaning; it is sometimes useful to define a data Thunk a = MkThunk a wrapper in order to introduce some intentional laziness. in our codebase, however, most of our fields are strict, so there is no semantic difference. another place where data makes more sense than newtype is with a record which happens to only have one field right now, but might have more in the future, as opposed to a wrapper type. I seem to have a different version of hlint than you, because my hlint report only finds one hint, not 3, so we only looked at ScopedEmpty, and that one does seem like it would benefit from being turned into a newtype. what are the other 2?
3. const and record patterns are both case by case, and we did not look at them individually, so I think we should leave them alone.

thanks for your interest in contributing to Klister!

[philderbeast]

Thanks @gelisam putting effort into this upfront, way more than I expected. Sorry if that was a drag on your time.

If it is OK by you, I'd prefer to tackle pull requests for hlint suggestions one suggestion at a time¹. To enable that, would it be OK to merge #234 for starters.

For the suggestions you consider bad, we can move them to their own section in .hlint.yaml like we did in liquid-fixpoint/.hlint.yaml

If you'd like a github action for this, we could do that by adding haskell-actions/hlint actions.

As for tackling a single suggestion, that would involve deleting a single - ignore ... line in .hlint.yaml and following the suggestions, sometimes using hlint --refactor² and sometimes doing it manually.

Footnotes

1. Keeps the changes small, the reviews lightweight and avoids cascading hlint suggestions where following one suggestion unveils a new suggestion. ↩

2. apply-refact can easily mess up, such as wiping out CPP conditionals or repeating comments. ↩

[philderbeast]

I seem to have a different version of hlint than you, because my hlint report only finds one hint

Yes that is likely. If we go for an action, the hlint version can be set there.

[gelisam]

Ah, so if we merge #234, hlint will not pester us about any of the suggestions listed there! I had misunderstood the purpose of that list, I thought it was the opposite. Sure, let's merge it!