Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modernise Keter #304

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Modernise Keter #304

wants to merge 6 commits into from

Conversation

jezen
Copy link
Collaborator

@jezen jezen commented Feb 12, 2025

This is a sweeping change which serves as a precursor to other necessary modernisation efforts. The commits are split by theme, and reading them that way is probably easier than viewing all the changes at once.

This change:

  • Switches the Nix configuration over to flakes
  • Runs nix flake check on CI, which lints, builds, and tests the entire project
  • Fixes all the linting errors
  • Removes support for a couple of [very] old libraries

Once this is merged, I can work on #258

I would like to have integrated tests in place before I fix other issues. The issues at the top of my hit list are

This is part of my work for Thaigersprint/thaigersprint-2025#1.


As a follow-up to this, I plan to remove support for anything legacy, and work towards cutting a new major version. I don't think it's worth supporting GHC <8, process < 1.6, and probably others.

This swaps the Nix configuration to use flakes and adds some checks that
we will use to try to keep the code a bit tidier and easier to maintain.
We'll run these checks in CI, and eventually add `nixosTest` for real
integrated testing.
This is part of the check that would be run on CI with

```
nix flake check -L
```

This isolates the formatting changes so the history of changes is easier
to follow.
This reformats all of the Markdown files which had messy formatting, and
now passes the markdownlint check.
Now all hlint and stylish-haskell have been fixed, and all flake checks
are passing.

One of the linters was failing to parse some of the files because of the
use of CPP for conditional imports. This is to maintain backwards
compatibility — which is of course a good thing — but I think it's the
wrong thing to do in this case.

1. I don't think it's worth maintaining backwards compatibility with
   http-reverse-proxy 0.4.2 which was released 10 years ago.

2. It's definitely not worth maintaining backwards compatibility with
   aeson < 2.0.1.0 since it fixed a hash collision vulnerability.
Now all linting, compiling, and testing will be done on CI.
@jezen
Copy link
Collaborator Author

jezen commented Feb 12, 2025

I would appreciate thoughts on this from:

I don't mean reading the code — the changes are mostly uninteresting. But the change I'm proposing is largely a philosophical one. Is it worth trying hard to support super old versions of compilers and libraries when there's little desire from anyone to work on this as it is? Personally, I don't think so. I think if keter is completely broken, then all people will really be motivated to do is find a tool which isn't completely broken. So, I think a stronger step towards a healthier tool is better than clinging to this brokenness in perpetuity.


If anyone thinks I'm wrong, and that a modernisation effort will actually cause more pain, please tell me. I don't want to break anyone else's setup (although given the above, I think this is unlikely anyway).

Copy link
Collaborator

@jappeace jappeace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have fun :)

statix.enable = true;
stylish-haskell.enable = true;
};
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of putting formatting checks in CI, but you're working on it so if it helps you go ahead.

Copy link
Contributor

@ulidtko ulidtko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... Beat me to it! 😆

I'd never finished neither published my own "mega-cleanup" keter branch. Super welcome, @jezen ! I'll gladly ditch all that & rebase patches after you merge your work.

It's indeed annoying to keep trying to not get distracted on minutiae, all the lints, formatting inconsistencies, chaos in imports — while trying to focus on some other issue. Had more than one branch die unfinished, bogged down in all that.

A special 👍 on ImportQualifiedPost. Language: GHC2021 might be worth considering, too.

I also find it mildly useful to do import grouping: separate project-local imports, from dependency imports[, from base/stdlib imports]. Simply with an empty line, stylish understands that. Good cross-language practice, too.

Another remark, on GHC <8 — please see https://endoflife.date/ghc 👀
We're almost squarely a decade (10 years) since 7.8 and 7.10 were the latest. Even assuming the most conservative stances, it's perfectly sensible to me to drop support for GHC <8 at this point.

I won't have objections to stop supporting GHC <9.2 even! If the compiler team recommends against using those, why would libraries carry the burden? And Keter isn't even a library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants