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

Add a code action to make all imports explicit #436

Merged
merged 1 commit into from
Sep 27, 2020

Conversation

pepeiborra
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@ndmitchell ndmitchell left a comment

Choose a reason for hiding this comment

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

Having one commit that is a lot of whitespace/formatting/naming changes, and also adds a feature, makes it pretty hard to review. That said, it looks pretty reasonable to me.

@alanz
Copy link
Collaborator

alanz commented Sep 27, 2020

Having one commit that is a lot of whitespace/formatting/naming changes

Agree on this. BUT, you can turn of whitespace changes in the diff, using the gear thing in the review

image

@pepeiborra
Copy link
Collaborator Author

Thanks! Apologies for the unnecessary review noise, I can't wait for a standard code formatting config

@lukel97
Copy link
Collaborator

lukel97 commented Oct 3, 2020

Adding this here for the changelog
explicit-import-code-action

@jrp2014
Copy link

jrp2014 commented Feb 6, 2021

@pepeiborra @alanz you may be interested in https://gitlab.haskell.org/ghc/ghc/-/issues/18247#note_283101

This makes some suggestions for improving ghc's getMinimalImports so that it works better with patterns, etc. The changes need validation by someone with a better understanding that I have of ghc's internals and their evolution over different versions of ghc, but it works for me. (In particular, the various to_ie functions need checking.) A complete version of the code is here: https://github.com/jrp2014/smuggler2/blob/master/src/Smuggler2/Imports.hs Ideally, when it's been check it would be merged into the ghc tree

@pepeiborra
Copy link
Collaborator Author

/cc @rayshih

@pepeiborra
Copy link
Collaborator Author

@jrp2014 you seem to have a working fix, but how do we know that it works? If I were you I would extend the GHC test suite with some unit tests that demonstrate the problems that you found, convince myself that it works for all cases, and then send a merge request.

If you have trouble convincing yourself, why not apply your changes to a subset of Hackage?

@jrp2014
Copy link

jrp2014 commented Feb 6, 2021

I have some test cases in the smuggler2 repo and I have tested on the Agda codebase, as close to a torture test as I could find, but someone the to_ie functions (which attempt to distinguish patterns from operators/symbols, etc) should be reviewed by someone knowledgeable as they were inferred by me from the documentation and so I may have missed something.

@pepeiborra
Copy link
Collaborator Author

Sending a merge request will accomplish that - get the relevant experts to look at your code and suggest improvements

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.

5 participants