Skip to content

Add qualified imports in postfix position when ImportQualifiedPost and WarnPrePositiveQualifiedModule are set #3399

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

Merged
merged 8 commits into from
Dec 27, 2022

Conversation

3kyro
Copy link
Contributor

@3kyro 3kyro commented Dec 14, 2022

With ImportQualifiedPost language extension and the -warnPrePositiveQualifiedModule enabled, the default behavior of the import-export plugin to add qualified imports in prefix position produces a warning. This PR changes the position of the qualified descriptor to postfix when the above two conditions are met. This is a partial fix for #3362.

@3kyro 3kyro force-pushed the postqualified-import-codeaction branch from dc27803 to 5699deb Compare December 15, 2022 10:06
@3kyro 3kyro force-pushed the postqualified-import-codeaction branch 2 times, most recently from 3a65034 to 5257dc7 Compare December 22, 2022 12:47
@3kyro 3kyro marked this pull request as ready for review December 23, 2022 07:55
@3kyro 3kyro requested a review from santiweight as a code owner December 23, 2022 07:55
@3kyro 3kyro force-pushed the postqualified-import-codeaction branch 5 times, most recently from 7bbb589 to e1f7507 Compare December 23, 2022 11:21
@3kyro 3kyro requested review from fendor and removed request for santiweight December 23, 2022 12:57
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, look great! Tell me if I am wrong, but it seems there is no test-case for the postfix position import?

@3kyro
Copy link
Contributor Author

3kyro commented Dec 24, 2022

@fendor thanks, I couldn't find a way to set -Wprepositive-qualified-module in the test environment and thought I might as well add an extra regression test. But it is obviously possible with the OPTIONS_GHC pragma 🤦🏽. I've added it now.

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, nice contribution!

The test fails with:

test\functional\FunctionalCodeAction.hs:116:
expected: "Not in scope: \8216Control.when\8217\nNo module named \8216Control\8217 is imported."
but got: "Variable not in scope: when :: Bool -> IO () -> IO ()"

Seems like diagnostics for missing variables differ
@3kyro 3kyro force-pushed the postqualified-import-codeaction branch from 841265d to 526221c Compare December 26, 2022 20:25
@fendor fendor added the merge me Label to trigger pull request merge label Dec 27, 2022
@mergify mergify bot merged commit d621fc4 into haskell:master Dec 27, 2022
@3kyro 3kyro deleted the postqualified-import-codeaction branch December 27, 2022 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants