Should we lint, -wall and format our code base?

[alanz]

I have recently spent some time in the code base again.

I can't help noticing that I have several hlint and other warnings, as well as a variety of formatting styles. Format differences naturally occur with multiple contributors of course.

Given the growing size and number of contributors, I think it would be good to enforce an hlint config, as well as require compiles to pass -wall, or some agreed set of warnings. This could initially be done via a separate "allow-fail" CI build which we aim to bring into compliance overtime.

And perhaps we should agree to use a specific formatter, and format all the code with it.

[konn]

I can't help noticing that I have several hlint and other warnings, as well as a variety of formatting styles. Format differences naturally occur with multiple contributors of course.

And perhaps we should agree to use a specific formatter, and format all the code with it.

Even though I'm not a maintainer, but as a spare-time contributor, I strongly agree with this point! While I tried to make some contributions to HLS, I have already broken the indent styles by accident for some times. The changes are trivial in some cases so that one can manually re-order import list or adjusting indentation, but sometimes it becomes rather tedious to maintain formatting coherency in per-module manner.

As a contributor, I feel more comfortable if there is project-wide formatter configuration and we can just let HLS to invoke them automatically. That saves essentially unnecessary works both for contributors and reviewers, reducing noise from reviwing process, IMHO.

[alanz]

Contentious question: which formatter?

To my mind it should be one without requiring much config, or used with the default config.

[jneira]

Contentious question: which formatter?

What about making a poll? 😄 I am personally fine with any of them, and even i am not too disturbed with the actual situation (although agree in it can save time for all of us). I've found more useful the more semantic oriented -Wall and hlint.

[Ailrun]

I think in terms of reducing config, ormolu would be a good choice.

[peterwicksstringfield]

Reformat the entire code base with whatever format is chosen, then re-run it on each PR before the unit tests. If there are changes they can be automatically committed and the build restarted. So any successful build is a fixed point of the formatter. I don't think it matters which format is chosen, just so long as the computer is the only one who has to actually think about it.

Wall and hlint sound good too.

[ndmitchell]

The formatting inconsistency in Ghcide is significantly higher than most code bases, because it went through 4 evolutions and 4 authors before it even became public. I think whatever we do here should also be done in Ghcide too.

As a general rule, I think the more authors work on a code base, the more stringent you should be with warnings. I don't like Wall for my personal projects, but for something with as many contributors as this project, it seems necessary. For formatting, I'd really like an endorsement from someone who has tried it in anger and found it worked. Fourmulu would be my default, but the number of end users is still lower than I'd like.

[georgefst]

Fourmulu would be my default, but the number of end users is still lower than I'd like.

Well, I don't really know how to measure that but I'm sure it's not huge. GitHub stars would put us at about 10% of the popularity of Ormolu, Stylish and Brittany. But it's likely to be higher than that since we haven't been around long and people aren't inclined to go back and un-star tools they've stopped using.

Anyway, I'm using it on pretty much all of my projects now, including a small team at work. And at least one other company is using it, based on one of our merged PRs. I've been keeping notes of places where I feel the output could be slightly nicer, but at this point they're mostly very minor things inherited from Ormolu. Main one is a slight over-keenness to insert line breaks (which I've rambled about a little in this issue). But some people presumably like the current behaviour, based on the popularity of Ormolu and Elm-format.

[ndmitchell]

Anyway, I'm using it on pretty much all of my projects now, including a small team at work.

To me, that's enough to recommend Fourmulu (or if some people prefer Ormulu, have a debate between the contributors). Real practical use by a contributor trumps most other things.

[konn]

+1 for fourmolu, as our team had switched to it a month ago and it seems really useful to minimise diffs on Git, and allows us additional configurations.

[Ailrun]

@konn I'm not sure whether it will be helpful to allow more configuration in this case. As mentioned before, Current HLS has lots of contributors and styles, and leaving a space for configuration may lead to some unproductive debates about what should be the style of HLS. I personally don't prefer Ormolu's (and similar) formatting style, but for some big project like this, I think their approach (un-configurability) is the right one.

[alanz]

I think configurability of the formatter or not is not critical, so long as we can have a project config for the chosen formatter, and everyone uses it.

[pepeiborra]

Hlint, ormolu, Brittany and stylish-haskell are supported by https://github.com/cachix/pre-commit-hooks.nix

[georgefst]

Hlint, ormolu, Brittany and stylish-haskell are supported by https://github.com/cachix/pre-commit-hooks.nix

Well, I'd been meaning to get around to it: cachix/pre-commit-hooks.nix#89

[Ailrun]

If there is no ohter opinion, let me try to configure Fourmolu (with default setting) for the project.

[Ailrun]

While I'm trying to configure Fourmolu, I noticed that we indeed need some configuration, but Fourmolu does not support directory-wide extension setting (like stylish-haskell does). Any idea? Maybe it's better to use stylish-haskell? (As it also uses ghc-lib-parser and supports such an extension setting)

[georgefst]

@Ailrun Is that something we really need? Ormolu/Fourmolu has most extensions enabled by default*. And HLS appears to use default-extensions sparingly, with extensions mostly enabled by per-file pragmas, which will be picked up by the formatter. Plus even the project-wide options will be picked up when running via HLS itself.

*The only omission I've run in to is TypeApplications, but perhaps this ought to change: tweag/ormolu#452.

[Ailrun]

GHCIDE code uses, for example, BangPatterns specified in .cabal file, and Fourmolu cannot format them without explicitly passing -XBangPatterns.

[georgefst]

Fair enough. I think we're likely to make that configurable via config file at some point in the (near?) future.

Meanwhile, just to check I'm not missing something: is there any context in which we expect passing the CLI flag to be much of a problem? I'd think that after the intial one-off formatting of the code base, we'd be expecting contributors to be formatting from within an editor with HLS running. Even if we add a CI check, the flags can be specified in the CI config.

[Ailrun]

As we want to set pre-commit-hook.nix too, we need to provide a proper option to .nix too, and those two option lists should be synced with the whole project which has several .cabal files. I think it's harder to manage than per-package config, which only needs to care about the related single .cabal file.

[tittoassini]

As a standard formatter, I would suggest to use formolu.

ormolu is not configurable and will insist to convert all multi line comment {- -} to single line ones and that will royally screw the tests for the eval plugin (I know that ormolu can be selectively disabled, but still).

[Ailrun]

@tittoassini please check the above discussion about Fourmolu/Ormolu and stylish-haskell.

[wz1000]

For the record, I prefer my code artisanally hand formatted, and I put a lot of thought into manually formatting it.

I've also never had any difficulty reading code written by well-intentioned humans due to formatting styles.

I will become quite cranky if a we start enforcing formatting for HLS/ghcide. I will become even more cranky if this contributes to endless merge conflicts in my big, long-lived PRs (like #1284). So I kindly request that no concrete action be taken on this issue until at least that PR is merged.

[Ailrun]

@wz1000 Actually, I'm done with basic setting, and just waiting for some big PRs to be merged (so that it does not cause myriad of conflicts, as you said) like #704, #1232, #1264 (which are now merged), and #1284.

[alanz]

For the record, I prefer my code artisanally hand formatted, and I put a lot of thought into manually formatting it.

@wz1000 I am generally in this camp too. But there have been some PRs where the author has formatting turned on, and they got very noisy. And some projects have good success with formatters.

My main concern is having the code compile with all warnings enabled, and no diagnostics.

[pepeiborra]

For what it's worth, I am entirely in favour of automated code formatting, regardless of the style.

[georgefst]

As it happens, I've shifted mostly in favour of automated formatting largely because of HLS itself - when you've got tooling inserting type signatures, imports, case expressions, record fields, linter suggestions etc. in to your code, it feels like a lot of effort to go back and manually sculpt them all in to your individual style.

[isovector]

1439 came around and broke a bunch of my weird vim macros that write code for me. I'd like to opt the tactics plugin out of these checks and large-scale refactors in the future, please.

[Ailrun]

If such a change breaks your macros, then some possible future PRs from other "formatter-on" contributors to the tactic plugin also will break those, right?

[isovector]

And I'll complain then too :) What it boils down to is that the cost of me rewriting the vim macros I've been using for seven years in order to appease some formatter's idea of what nice imports look like is too high. I'm all for however you'd all like to format HLS proper, but tactics is a single person codebase that just happens to live in this repository for convenience.

[Ailrun]

I'm not against disabling the hook and formatter for the tactic plugin. My only concern is just that even without our official recommendations, some contributors will use formatters, and since we don't know what kind of formatting may break your macros, we cannot approve any PRs for the tactic plugin that change some styles and need to wait your approval.

[isovector]

I'm happy to cross that bridge when we come to it. Sorry for being a nuisance on this, but I'd really appreciate it if we can disable the formatter hook for tactics.

[Ailrun]

Sure, I will update the hook.

[isovector]

Thanks so much. Greatly appreciated!

[jneira]

We are already applying hlint, a formatter, etc, maybe it is time to close this generic issue and open new ones for specific aspects of the process