Skip to content

Make explicit import suggestions more consistent#3142

Merged
kritzcreek merged 2 commits intopurescript:masterfrom
nwolverson:import-suggestion-dotdot
Jan 20, 2018
Merged

Make explicit import suggestions more consistent#3142
kritzcreek merged 2 commits intopurescript:masterfrom
nwolverson:import-suggestion-dotdot

Conversation

@nwolverson
Copy link
Copy Markdown
Contributor

@nwolverson nwolverson commented Oct 31, 2017

It's been annoying me that while either psc-ide or the suggestions for ImplicitImport/HidingImport will suggest Blah(..) for the import of a type and its data constructors, but other import suggestions always suggest the explicitly listed form Blah(Blah1, Blah2,...), so that the suggested imports change even when they are not relevant to the warning.

e.g. when I have used Just

Module Foo has unspecified imports, consider using the explicit form:

    import Foo (Maybe(..))

vs

The import of module Foo contains the following unused references:

    zzz

  It could be replaced with:

    import Foo (Maybe(Just))

This change instead uses the (..) form if that was used previously, but still maintains the form with explicitly listed data constructors if present. So where I have an import import Foo (Either(..), Maybe(Just, Nothing), zzz)

The import of module Foo contains the following unused references:

    zzz

  It could be replaced with:

    import Foo (Either(..), Maybe(Just))

(I'd like to test this, but as far as I know existing warnings tests are just testing the error code?)

@nwolverson
Copy link
Copy Markdown
Contributor Author

nwolverson commented Nov 19, 2017

Uh. I guess I thought this got merged, not sure how it shows as having 32 commits. I guess this needs rebased?

@hdgarrood
Copy link
Copy Markdown
Contributor

hdgarrood commented Dec 15, 2017

I wonder if this commit history has something to do with the 0.12 branch... I'll try to work out what's going on with that and get back to you.

@nwolverson
Copy link
Copy Markdown
Contributor Author

nwolverson commented Dec 15, 2017

Wasn't sure what happened with the 0.12 branch, seemed like some funny business - I can easily enough cherry pick the commits back on top of master. I guess it was ac18e53 80b04b0

@nwolverson nwolverson force-pushed the import-suggestion-dotdot branch from 80b04b0 to 41bc81a Compare December 15, 2017 20:57
@nwolverson
Copy link
Copy Markdown
Contributor Author

nwolverson commented Dec 15, 2017

Actually seems there was only 1 commit for this PR, I think this should be back in a sensible state.

@hdgarrood
Copy link
Copy Markdown
Contributor

hdgarrood commented Dec 16, 2017

I tested this manually with the following input source files, comparing the behaviour on master with this PR: https://gist.github.com/hdgarrood/a0d2b03f6af2b73ba6c376e4be1aa4ab

This seems to work great; there's just one thing I noticed:

module ImplicitQualifiedImport where

import Prelude
import Data.Array as M
import Data.Maybe as M

foo :: M.Maybe Unit
foo = M.Just unit

suggests using import Data.Maybe (Maybe(Just)) as M, whereas the ImplicitImport warning suggests using the (..) form; presumably these should both do the same thing too? I guess they should both suggest (..)?

@garyb
Copy link
Copy Markdown
Member

garyb commented Dec 16, 2017

I guess they should both suggest (..)?

👍

@nwolverson
Copy link
Copy Markdown
Contributor Author

nwolverson commented Dec 17, 2017

Good spot, the ImplicitQualifiedImport suggestion should now be consistent with the others.

Something I did test before and I think was missing from that gist, is checking the "maintains the form with explicitly listed data constructors if present" - ie. the case of

import Data.Maybe (Maybe(Just), fromMaybe)

when Just is used but not fromMaybe, the suggestion to remove fromMaybe should not alter Maybe(Just), i.e. is

import Data.Maybe (Maybe(Just))

Updated gist here https://gist.github.com/nwolverson/f94772a141c68178acb09b68f4d41cbd

hdgarrood
hdgarrood approved these changes Dec 17, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@hdgarrood hdgarrood left a comment

Choose a reason for hiding this comment

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

Ah yes, of course, thanks :)

I think this will be really nice to have! I'm happy with how this has been tested, but I'd appreciate if someone who is a little more familiar with this code could look over it before we merge, since I'm not very familiar with it, really.

@LiamGoodacre
Copy link
Copy Markdown
Member

LiamGoodacre commented Dec 25, 2017

(I'm also not familiar with this part of the code, but this looks good 👍)

@hdgarrood
Copy link
Copy Markdown
Contributor

hdgarrood commented Dec 26, 2017

@garyb would you like to look over this? No rush, of course.

@garyb
Copy link
Copy Markdown
Member

garyb commented Dec 29, 2017

Sure thing. I'm taking a mostly-complete break from code stuff just now, but will definitely take a look in the new year :)

@kritzcreek
Copy link
Copy Markdown
Member

kritzcreek commented Jan 20, 2018

I've built and tested this and it works nicely, thanks!

@kritzcreek kritzcreek merged commit 3679da4 into purescript:master Jan 20, 2018
This was referenced Apr 29, 2018
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@hdgarrood hdgarrood hdgarrood approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@nwolverson @hdgarrood @garyb @LiamGoodacre @kritzcreek