Autoformat more directories

[9999years]

sickos yes

[ulysses4ever]

bold! People seem to want to scrap auto-formatting as it creates a noticeable tension with new/rare contributors.

One thing to consider is the release cycle. We're about to release cabal-install-3.14.1.0, and there will be many backports, which reformatting may complicate. A better point for it may be a little while after a release.

[geekosaur]

That's likely to feel like 3.10 after the initial fourmolu pass, though.

[9999years]

auto-formatting [...] creates a noticeable tension with new/rare contributors

FWIW I was very surprised to hear this -- I find an autoformatter that clearly fails in CI so much easier to work with as an infrequent contributor than a (usually vague) style guide that's variably enforced by individual reviewers...

[philderbeast]

I'd really like to see this go in. I don't want have to bother formatting code by hand.

[philderbeast]

@9999years, I can help with the CPP drudgery if that would speed things up.

[ulysses4ever]

bold! People seem to want to scrap auto-formatting as it creates a noticeable tension with new/rare contributors.

Since we turned 180 on scrapping the autoformatter (again!) and seem to be sticking with it, I'd be fine with this patch (once it's turned out of the draft state), in principle.

Is it intentional that it doesn't touch the CI? CI currently does its own thing with the action, and has its own list of directories, sadly. I'd be happier if CI exercised the make-targets. But I also foresee some contributors screaming at CI if it starts checking tests, for instance...

[9999years]

@ulysses4ever Definitely not intentional, good catch!

[ulysses4ever]

Case seems off here (and maybe somewhere else)? I mean Cabal-testsuite not cabal-testsuite, and the latter is the correct one.

It's also strange that CI missed this issue.

[9999years]

@alt-romes @mpickering Do you have any PRs which would conflict with this? Happy to split this PR up.

[9999years]

From today's meeting: We might want to pay attention to: #9743

[ulysses4ever]

To expand on @9999years's comment above, we're particularly worried about #9743 But as discussed at the meeting, the PR branch could be reformatted accordingly, and then it should hopefully not require any other effort.

[philderbeast]

If we're worried about conflicts, we could merge #10683 early that deals with the parsing errors fourmolu encounters, then merge other major pull requests that we don't want to reformat and then merge this pull request.

[mpickering]

I don't think that a autoformatter should be used at all, so splitting up the PR or anything else won't help so much. I thought the consensus was to remove the formatter and it seems that a single PR has changed the course again?

There will always be long-lived branches for large open-source projects which become hard to rebase, it's not a temporary problem.

[9999years]

• Fixed the directory name case issues
• Incorporated the comment changes in Add make style-todo and fourmolu on off comments #10683 (which will disappear from this PR once that is merged)
• Split this into multiple commits -- one to add the directories to the Makefile and GitHub Actions and one to actually run the formatter

[9999years]

There will always be long-lived branches for large open-source projects which become hard to rebase, it's not a temporary problem.

@mpickering The current plan is to autoformat the LTS branches to match this PR, once this PR is merged, so that backports will remain easy. Does that help ameliorate your concerns?

[mpickering]

@9999years No, that does not address the concern at all. The concern is that there are long-lived contributor branches for complicated features which may exists for years outside the tree before being merged.

[ulysses4ever]

@mpickering you can reformat the long-lived branch any time, if the "base" branch you want to merge into was reformatted over time. And the result would be the diff we are interested in, nothing about formatting. I still don't see any problem. What am I missing?

[9999years]

... there are long-lived contributor branches for complicated features which may exists for years outside the tree before being merged ...

This seems to me to be a symptom of an issue. Big changes like that should be shipped incrementally. Large complicated features (and large complicated diffs) are essentially impossible to review and should be discouraged. Even without reformatting the codebase, regular churn means constant rewrites to adjust to the underlying architecture shifting. (See, for example, how I had to completely rewrite #9367 before it could be merged, because the way Cabal runs ghc changed between @Gabriella439's initial draft and the final approvals. And that was for a ~100-line diff!)

[mpickering]

Yes, I think we certainly agree that working on a long lived patch is difficult on a large codebase. Rebasing after a period is challenging due to the churn from changes. It seems like we disagree on taking measures to help that situation by removing unnecessary formatting changes from the diff.

It does seem better to be able to incrementally land patches, but my experience is that making incremental changes can be difficult in an open-source project, as there is no guarantee anyone will finish a patch. It is easily possible that things are left in an intermediate state for a long period, and maintainers don't feel like they have agency to revert partial changes.

If the maintainers of cabal are in agreement that a formatter is a good idea, then it is not the place to litigate on each PR about the usage of a formatter. It was just my understanding that they had already decided to remove the formatter requirement. I know I am not the only contributor who has found the formatter gets in the way of development, so if on balance the maintainers wish to take these complaints into account but decide on balance it is worthwhile then I will respectfully disagree and direct my energy into writing productive patches.

[9999years]

... my experience is that making incremental changes can be difficult in an open-source project, as there is no guarantee anyone will finish a patch. It is easily possible that things are left in an intermediate state for a long period, and maintainers don't feel like they have agency to revert partial changes.

Yeah, I definitely agree that this is a problem to work on — I think the maintainers should feel comfortable reverting partial changes. I also think that maintainers should have more power with reverting those changes. I think two-reviews-per-PR keeps the velocity of this project much slower than it needs to be and discourages splitting up changes into multiple PRs (because getting two reviews is, in my experience, a lot harder than getting just one).

[Gabriella439]

In my experience, formatting changes don't make it significantly harder to rebase a PR. Like, yeah, they can't be resolved trivially, but usually it's pretty obvious how to adapt a rebased commit to accommodate a refactor-related change on master. The thing that makes a PR really hard to rebase is semantic changes or refactors on master, not formatting changes.