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

Actually enable pedantic flag in ci flags job #4224

Merged
merged 4 commits into from
May 13, 2024
Merged

Conversation

jhrcek
Copy link
Collaborator

@jhrcek jhrcek commented May 12, 2024

As found in #4223 we're not actually enabling pedantic in CI at all.
This enables it for all cabal projects for which most warnings have been fixed so far (not yet for ghcide).

@jhrcek jhrcek marked this pull request as ready for review May 12, 2024 11:37
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM, some nitpicks

plugins/hls-change-type-signature-plugin/test/Main.hs Outdated Show resolved Hide resolved
plugins/hls-class-plugin/test/Main.hs Outdated Show resolved Hide resolved
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

One thing I noticed:

haskell-language-server.cabal Outdated Show resolved Hide resolved
--constraint "ghcide +ekg +executable +test-exe" \
--constraint "hls-plugin-api -use-fingertree" \
--constraint "all +pedantic"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This all +pedantic did not have any effect - it needs to be enabled on per-cabal-file basis.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, do we expect this to work? Is there a cabal ticket discussing this behaviour?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't open any ticket about this in cabal, nor did I research why it doesn't work/whether it should work at all.
I just mistakenly assumed this works when I refactored this job.
Let me ask about it in cabal issue tracker later today.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It turns out that the way to enable the flag for all local packages is via
cabal configure --flag pedantic

I'll make use of that in upcoming PR after I fix warnings in ghcide

@jhrcek jhrcek requested a review from fendor May 13, 2024 07:56
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM

@michaelpj michaelpj merged commit 4985793 into master May 13, 2024
39 checks passed
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