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

Fix/remove warnings and general cleanup #1862

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

Conversation

Vlix
Copy link
Contributor

@Vlix Vlix commented Mar 4, 2025

Before submitting your PR, check that you've:

  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

Prompted by #1852

Added a comment on that PR, but wanted to double check I was right, and one thing led to another. Mostly me being annoyed by the messy code, so I did some cleanup that removed almost all the warnings when building with LTS-22 at least.

I want to check that all the builds pass (and if not fix it, obviously) and make sure most warnings are gone with any of the GHC versions that are tested.

FOR ANYONE REVIEWING

I've made the commits pretty self-contained, so if you go through the commits one by one, the review process should be pretty easy and quick.
The changes should retain function parity, and nothing should inherently change in behaviour.

@Vlix
Copy link
Contributor Author

Vlix commented Mar 4, 2025

Only failing job is because of external issues.

I'll be going through the logs to find some more warnings to get rid of. Consider this PR a "WIP" until then.

@Vlix Vlix marked this pull request as draft March 4, 2025 01:23
@Vlix
Copy link
Contributor Author

Vlix commented Mar 4, 2025

Builds except for the following:

  • The lts-23 jobs all failed because of hash oopsies before the yesod code even ran, so those should be retried, I think?
  • The windows-latest lts-22 job failed on a permission denied issue when building happy (trying to use strip.exe, known issue IIRC, not sure why it pops up now, though)

@Vlix Vlix marked this pull request as ready for review March 4, 2025 23:49
@jezen
Copy link
Member

jezen commented Mar 5, 2025

In general, I'm a big fan of the cleanup. Are you using tools like stylish-haskell and/or hlint to guide your changes?

@Vlix
Copy link
Contributor Author

Vlix commented Mar 5, 2025

I've done everything manually. Building the entire repo with --ghc-options -Wall and going to every spot which gave a warning one by one and fixing it up (or leaving some that seemed reasonable or too much hassle to "fix"). And most of the modules I touched, I also ordered the pragmas, because the inconsistency was annoying me.

If we'd want to organize all the modules, it'd be better to just throw an ormolu/fourmolu over it in one go as a single PR. Though it would also probably create conflicts in every open PR.

@jezen
Copy link
Member

jezen commented Mar 7, 2025

If we'd want to organize all the modules, it'd be better to just throw an ormolu/fourmolu over it in one go as a single PR. Though it would also probably create conflicts in every open PR.

Personally, I don't think we should do that.


Let's retry the failing CI checks.

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.

2 participants