Skip to content

handle trailing comma in import list properly #3035

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 11 commits into from
Jul 16, 2022
Merged

Conversation

kokobd
Copy link
Collaborator

@kokobd kokobd commented Jul 11, 2022

fix #2958

@kokobd kokobd force-pushed the kokobd/fix-import-2958 branch from 3fc1062 to 3393450 Compare July 12, 2022 03:34
@kokobd kokobd force-pushed the kokobd/fix-import-2958 branch from 3535077 to 20e73a4 Compare July 12, 2022 04:47
@kokobd
Copy link
Collaborator Author

kokobd commented Jul 12, 2022

Strange. I can not reproduce this test failure locally. Both running tests and manually trying this feature give successful results.

extend multi line import with trailing comma:                                                     FAIL (1.67s)
          test/exe/Main.hs:[208](https://github.com/haskell/haskell-language-server/runs/7295318339?check_suite_focus=true#step:8:209)3:
          expected: "module ModuleB where\nimport ModuleA (stuffB, stuffA,\n               )\nmain = print (stuffA, stuffB)\n"
           but got: "module ModuleB where\nimport ModuleA (stuffB, stuffA\n               )\nmain = print (stuffA, stuffB)\n"
          Use -p '/without checkAll.extend multi line import with trailing comma/' to rerun this test only.

@kokobd kokobd marked this pull request as ready for review July 12, 2022 22:56
@kokobd kokobd requested a review from pepeiborra as a code owner July 12, 2022 22:56
@kokobd
Copy link
Collaborator Author

kokobd commented Jul 12, 2022

I can not reproduce this test failure locally.

Okay, that's because ghc-exactprint behaves differently in GHC 9.0 (not 9.2!) and GHC 8.10.7, where the API is the same. But in GHC 9.0, an extra comma is attached to the "list", besides the one attached to the "list item". According to the docs, that behavior might be wrong.

Anyway, I finally got all compatibility issues handled.

@kokobd kokobd requested review from georgefst, michaelpj and wz1000 July 13, 2022 03:36
@georgefst
Copy link
Collaborator

Works for me on GHC 9.2.3, thanks! (but I won't approve this because I haven't looked at the code)

@kokobd
Copy link
Collaborator Author

kokobd commented Jul 13, 2022

Works for me on GHC 9.2.3

@georgefst Thanks for your testing!

@kokobd kokobd requested review from guibou and fendor July 14, 2022 11:58
kokobd added 2 commits July 14, 2022 13:21
These cache directories are small, but not preserving them requires
HLS to compile all modules in local project on workspace restarts.
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!

Comment on lines 383 to 384
let isAnnComma (G AnnComma, _) = True
isAnnComma _ = False
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: can we align this or remove the superfuous space?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Aha, my mistake. btw, stylish-haskell refuses to run on this file, because there are too many CPP, making it fail.

@kokobd kokobd added the merge me Label to trigger pull request merge label Jul 16, 2022
@mergify mergify bot merged commit 2f886bf into master Jul 16, 2022
@kokobd kokobd deleted the kokobd/fix-import-2958 branch July 16, 2022 11:46
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.

Parse error when adding def to trailing-comma import list Adding imports adds an extra comma
3 participants