Skip to content

Add a code action to make all imports explicit#436

Merged
pepeiborra merged 1 commit into
masterfrom
explicit-imports-code-action
Sep 27, 2020
Merged

Add a code action to make all imports explicit#436
pepeiborra merged 1 commit into
masterfrom
explicit-imports-code-action

Conversation

@pepeiborra

Copy link
Copy Markdown
Collaborator

No description provided.

@ndmitchell ndmitchell left a comment

Copy link
Copy Markdown
Collaborator

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

alanz commented Sep 27, 2020

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator Author

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

@lukel97

lukel97 commented Oct 3, 2020

Copy link
Copy Markdown
Collaborator

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

@jrp2014

jrp2014 commented Feb 6, 2021

Copy link
Copy Markdown

@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
Copy Markdown
Collaborator Author

/cc @rayshih

@pepeiborra

Copy link
Copy Markdown
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

jrp2014 commented Feb 6, 2021

Copy link
Copy Markdown

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
Copy Markdown
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