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.

Sign up for GitHub

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

Jump to bottom

Too many import lists: let's make names unique and drop same-package import lists #2835

Closed
ezyang opened this issue Sep 24, 2015 · 9 comments · Fixed by #3018
Closed

Too many import lists: let's make names unique and drop same-package import lists #2835

ezyang opened this issue Sep 24, 2015 · 9 comments · Fixed by #3018
Assignees
@ezyang
Labels
Cabal: other cabal-install: other

Comments

@ezyang
Copy link
Contributor

ezyang commented Sep 24, 2015

I've recently done several refactors on the Cabal code base (moving around some core fields in order to make things cleaner, etc) and I've noticed the single biggest annoying thing about doing this is fixing up the import lists. EVERY SINGLE TIME. It is driving me completely batty, and wasting so much of my time.

I want to kill it with fire. Specifically:

  1. I'll delete every explicit import list in Cabal.
  2. Whenever there is a conflict, I'll disambiguate by renaming the identifiers to something unique, so that given any name, there is exactly one definition in Cabal.

I want some people to agree that we should do this, and then I'll do it.
@gbaz
Copy link
Collaborator

gbaz commented Sep 24, 2015

I also don't like the explicit imports everywhere style, found it irritating to work with, and am cool with this. But I am only an occasional cabal hacker, so I conformed to convention. This sort of thing should be decided by those who most frequently hack on the code since it matters most to their quality of life, so my opinion doesn't matter much here. That said, 👍

@23Skidoo
Copy link
Member

I find them useful for code navigation. M-<; C-f foo tells you where foo came from.

Whenever there is a conflict, I'll disambiguate by renaming the identifiers to something unique.

Can be controversial for data structure-like things, e.g. Foo.{new,foldWithKey} vs newFoo,foldFooWithKey.

/cc @dcoutts

@ezyang
Copy link
Contributor Author

ezyang commented Sep 24, 2015

@23Skidoo I personally use hasktags to do code navigation. This works really great for same-project since the tags file is then pretty comprehensive.

If it's data structure like, I expect that it will be imported qualified and won't induce a conflict rename.

@ttuegel
Copy link
Member

ttuegel commented Sep 24, 2015

Can be controversial for data structure-like things, e.g. Foo.{new,foldWithKey} vs newFoo,foldFooWithKey.

In principle, I agree with disambiguating names wherever practical, but I don't want to see us in a situation like this.

@ezyang
Copy link
Contributor Author

ezyang commented Sep 24, 2015

@ttuegel To be clear, I am NOT proposing to rename data structure names. (GHC does have the qualified style but I think it's uncommon in the broader library ecosystem)

@beni55 I don't understand your comment.

@tibbe
Copy link
Member

tibbe commented Sep 24, 2015

No, we should use namespaces and not ad-hoc prefixes. One way to make this less annoying is to just import qualified and then use Module.foo.

@ezyang
Copy link
Contributor Author

ezyang commented Sep 30, 2015

OK, so I guess then an alternate proposal is to:

  1. Delete every explicit import list in Cabal
  2. For every conflict, explicitly qualify one (or more) of the conflicting modules

@ttuegel
Copy link
Member

ttuegel commented Sep 30, 2015

👍 I think that will already be a big improvement.

@ezyang ezyang self-assigned this Dec 23, 2015
ezyang added a commit to ezyang/cabal that referenced this issue Jan 2, 2016
@ezyang
Remove same-package import lists, fixes haskell#2835 for Cabal.
61674cf 
Signed-off-by: Edward Z. Yang <[email protected]>
ezyang added a commit to ezyang/cabal that referenced this issue Jan 3, 2016
@ezyang
Remove same-package import lists, fixes haskell#2835.
4993907 
Instead of qualifying, in some cases I just added an extra
'hiding' pragma to squelch errors.

Signed-off-by: Edward Z. Yang <[email protected]>
ezyang added a commit to ezyang/cabal that referenced this issue Jan 3, 2016
@ezyang
Remove same-package import lists, fixes haskell#2835.
d2d1bc8 
Instead of qualifying, in some cases I just added an extra
'hiding' pragma to squelch errors.  Common conflicts
(just grep for hiding):

    - Flag
        - Distribution.Simple.Compiler
        - Distribution.PackageDescription
        - Distribution.Simple.Setup
    - installedComponentId
        - Distribution.Package
        - Distribution.InstalledPackageInfo
    - doesFileExist
        - Distribution.PackageDescription.Check
    - instantiatedWith
        - I renamed the field in InstalledPackageInfo.  But
          it's probably going away with Backpack bits; I
          migth just excise it soon.
    - absoluteInstallDirs and substPathTemplate
        - Distribution.Simple.InstallDirs
        - Distribution.Simple.LocalBuildInfo

I fixed some shadowing errors by renaming local variables in some cases.
Common shadowings (we should perhaps consider not using these as
method/field names):

    - freeVars
    - components
    - noVersion
    - verbosity
    - get
    - description
    - name

Some data structures were moved around (IPIConvert and ABIHash)
to make it easier to handle import lists.

Some functions in Utils could be turned into reexports of standard
library functions.

No explicit imports were removed from non-Cabal imports.  These
imports help maintain warning cleanliness across versions of GHC,
so I don't recommend removing them.

Signed-off-by: Edward Z. Yang <[email protected]>
@ezyang ezyang mentioned this issue Jan 3, 2016
Merged
ezyang added a commit to ezyang/cabal that referenced this issue Jan 3, 2016
@ezyang
Remove same-package import lists, fixes haskell#2835.
b50b325 
Instead of qualifying, in some cases I just added an extra
'hiding' pragma to squelch errors.  Common conflicts
(just grep for hiding):

    - Flag
        - Distribution.Simple.Compiler
        - Distribution.PackageDescription
        - Distribution.Simple.Setup
    - installedComponentId
        - Distribution.Package
        - Distribution.InstalledPackageInfo
    - doesFileExist
        - Distribution.PackageDescription.Check
    - instantiatedWith
        - I renamed the field in InstalledPackageInfo.  But
          it's probably going away with Backpack bits; I
          migth just excise it soon.
    - absoluteInstallDirs and substPathTemplate
        - Distribution.Simple.InstallDirs
        - Distribution.Simple.LocalBuildInfo

I fixed some shadowing errors by renaming local variables in some cases.
Common shadowings (we should perhaps consider not using these as
method/field names):

    - freeVars
    - components
    - noVersion
    - verbosity
    - get
    - description
    - name

Some data structures were moved around (IPIConvert and ABIHash)
to make it easier to handle import lists.

Some functions in Utils could be turned into reexports of standard
library functions.

No explicit imports were removed from non-Cabal imports.  These
imports help maintain warning cleanliness across versions of GHC,
so I don't recommend removing them.

Signed-off-by: Edward Z. Yang <[email protected]>
ezyang added a commit to ezyang/cabal that referenced this issue Jan 8, 2016
@ezyang
Remove same-package import lists, fixes haskell#2835.
639cd00 
Instead of qualifying, in some cases I just added an extra
'hiding' pragma to squelch errors.  Common conflicts
(just grep for hiding):

    - Flag
        - Distribution.Simple.Compiler
        - Distribution.PackageDescription
        - Distribution.Simple.Setup
    - installedComponentId
        - Distribution.Package
        - Distribution.InstalledPackageInfo
    - doesFileExist
        - Distribution.PackageDescription.Check
    - instantiatedWith
        - I renamed the field in InstalledPackageInfo.  But
          it's probably going away with Backpack bits; I
          migth just excise it soon.
    - absoluteInstallDirs and substPathTemplate
        - Distribution.Simple.InstallDirs
        - Distribution.Simple.LocalBuildInfo

I fixed some shadowing errors by renaming local variables in some cases.
Common shadowings (we should perhaps consider not using these as
method/field names):

    - freeVars
    - components
    - noVersion
    - verbosity
    - get
    - description
    - name

Some data structures were moved around (IPIConvert and ABIHash)
to make it easier to handle import lists.

Some functions in Utils could be turned into reexports of standard
library functions.

No explicit imports were removed from non-Cabal imports.  These
imports help maintain warning cleanliness across versions of GHC,
so I don't recommend removing them.

Signed-off-by: Edward Z. Yang <[email protected]>
@ezyang
Copy link
Contributor Author

ezyang commented Jan 8, 2016

We probably should also do this for cabal-install but I'll leave that for another time.

garetxe pushed a commit to garetxe/cabal that referenced this issue Jan 9, 2016
@ezyang
Remove same-package import lists, fixes haskell#2835.
eebea92 
Instead of qualifying, in some cases I just added an extra
'hiding' pragma to squelch errors.  Common conflicts
(just grep for hiding):

    - Flag
        - Distribution.Simple.Compiler
        - Distribution.PackageDescription
        - Distribution.Simple.Setup
    - installedComponentId
        - Distribution.Package
        - Distribution.InstalledPackageInfo
    - doesFileExist
        - Distribution.PackageDescription.Check
    - instantiatedWith
        - I renamed the field in InstalledPackageInfo.  But
          it's probably going away with Backpack bits; I
          migth just excise it soon.
    - absoluteInstallDirs and substPathTemplate
        - Distribution.Simple.InstallDirs
        - Distribution.Simple.LocalBuildInfo

I fixed some shadowing errors by renaming local variables in some cases.
Common shadowings (we should perhaps consider not using these as
method/field names):

    - freeVars
    - components
    - noVersion
    - verbosity
    - get
    - description
    - name

Some data structures were moved around (IPIConvert and ABIHash)
to make it easier to handle import lists.

Some functions in Utils could be turned into reexports of standard
library functions.

No explicit imports were removed from non-Cabal imports.  These
imports help maintain warning cleanliness across versions of GHC,
so I don't recommend removing them.

Signed-off-by: Edward Z. Yang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ezyang ezyang

Labels
Cabal: other cabal-install: other
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove same-package import lists, fixes #2835 for Cabal. ezyang/cabal
6 participants
@ezyang @tibbe @23Skidoo @ttuegel @gbaz and others