Add 'imported-from' command. #823

Open
wants to merge 36 commits into
from

Conversation

Projects
None yet
3 participants
@DanielG
Owner

DanielG commented Aug 9, 2016

No description provided.

@DanielG DanielG added this to the v5.7.0.0 milestone Aug 9, 2016

@carlohamalainen

This comment has been minimized.

Show comment
Hide comment
@carlohamalainen

carlohamalainen Aug 9, 2016

Collaborator

So I'm right to clone the main repo and push directly to this branch?

Collaborator

carlohamalainen commented Aug 9, 2016

So I'm right to clone the main repo and push directly to this branch?

@DanielG

This comment has been minimized.

Show comment
Hide comment
@DanielG

DanielG Aug 9, 2016

Owner

Yeah, you can just add this repo as a remote to your existing repo though, no need to re-clone.

Owner

DanielG commented Aug 9, 2016

Yeah, you can just add this repo as a remote to your existing repo though, no need to re-clone.

@lierdakil

This comment has been minimized.

Show comment
Hide comment
@lierdakil

lierdakil Aug 9, 2016

Collaborator

WRT tests, I should probably tweak test code so it fails early, but here's an actual problem:

>>>> Cabal errors begin
Warning: /home/travis/build/DanielG/ghc-mod/ghc-mod.cabal: Ignoring unknown
section type: custom-setup
cabal: Could not resolve dependencies:
trying: ghc-mod-5.6.0.0 (user goal)
trying: base-4.8.2.0/installed-0d6... (dependency of ghc-mod-5.6.0.0)
next goal: haddock-api (dependency of ghc-mod-5.6.0.0)
rejecting: haddock-api-2.17.3, 2.17.2, 2.16.1, 2.16.0 (conflict: ghc-mod =>
haddock-api<2.16)
rejecting: haddock-api-2.15.0.2, 2.15.0.1, 2.15.0 (conflict:
base==4.8.2.0/installed-0d6..., haddock-api => base>=4.3 && <4.8)
Dependency tree exhaustively searched.
<<<< Cabal errors end
Collaborator

lierdakil commented Aug 9, 2016

WRT tests, I should probably tweak test code so it fails early, but here's an actual problem:

>>>> Cabal errors begin
Warning: /home/travis/build/DanielG/ghc-mod/ghc-mod.cabal: Ignoring unknown
section type: custom-setup
cabal: Could not resolve dependencies:
trying: ghc-mod-5.6.0.0 (user goal)
trying: base-4.8.2.0/installed-0d6... (dependency of ghc-mod-5.6.0.0)
next goal: haddock-api (dependency of ghc-mod-5.6.0.0)
rejecting: haddock-api-2.17.3, 2.17.2, 2.16.1, 2.16.0 (conflict: ghc-mod =>
haddock-api<2.16)
rejecting: haddock-api-2.15.0.2, 2.15.0.1, 2.15.0 (conflict:
base==4.8.2.0/installed-0d6..., haddock-api => base>=4.3 && <4.8)
Dependency tree exhaustively searched.
<<<< Cabal errors end
@lierdakil

This comment has been minimized.

Show comment
Hide comment
@lierdakil

lierdakil Aug 9, 2016

Collaborator

Okay, 7.10 fixed. 7.8 is still failing, in part due to test files not compiling under 7.8 (f.ex. ImportedFrom03.hs has some instance-related fails), and in part I honestly don't know why, but it throws GMENoVisibleExports.

Collaborator

lierdakil commented Aug 9, 2016

Okay, 7.10 fixed. 7.8 is still failing, in part due to test files not compiling under 7.8 (f.ex. ImportedFrom03.hs has some instance-related fails), and in part I honestly don't know why, but it throws GMENoVisibleExports.

@carlohamalainen

This comment has been minimized.

Show comment
Hide comment
@carlohamalainen

carlohamalainen Aug 9, 2016

Collaborator

The failures on 7.8.4 are my fault, have reproduced them on my laptop. I'll look into it.

Collaborator

carlohamalainen commented Aug 9, 2016

The failures on 7.8.4 are my fault, have reproduced them on my laptop. I'll look into it.

@carlohamalainen

This comment has been minimized.

Show comment
Hide comment
@carlohamalainen

carlohamalainen Aug 15, 2016

Collaborator

@DanielG I've gone through all your comments, happy for final review now.

Collaborator

carlohamalainen commented Aug 15, 2016

@DanielG I've gone through all your comments, happy for final review now.

@DanielG

This comment has been minimized.

Show comment
Hide comment
@DanielG

DanielG Aug 27, 2016

Owner

I'm doing a cleanup pass right now so don't bother fixing any of the things I'm about to comment on. Those are just FYI.

Owner

DanielG commented Aug 27, 2016

I'm doing a cleanup pass right now so don't bother fixing any of the things I'm about to comment on. Those are just FYI.

Language/Haskell/GhcMod/ImportedFrom.hs
+refineAs (MySymbolUserQualified userQualSym) exports = filterM f exports
+ where
+ -- f :: ModuleExports -> Bool
+ f export = case modas of

This comment has been minimized.

@DanielG

DanielG Aug 27, 2016

Owner

I replaced this entire function with modAs == moduleOfQualifiedName userQualSym. Don't you think a datatype called MySymbolUserQualified should maintain the invariant that the module it contains is actually qualified ;)

The way we deal with this usually is by using smart constructors (mkSymbolUserQualified etc.) that check this invariant and only use those to construct that type. (optionally moving the type definition to a separate module that doesn't export the constructor so the mkFun in the only way to make a value of that type.

@DanielG

DanielG Aug 27, 2016

Owner

I replaced this entire function with modAs == moduleOfQualifiedName userQualSym. Don't you think a datatype called MySymbolUserQualified should maintain the invariant that the module it contains is actually qualified ;)

The way we deal with this usually is by using smart constructors (mkSymbolUserQualified etc.) that check this invariant and only use those to construct that type. (optionally moving the type definition to a separate module that doesn't export the constructor so the mkFun in the only way to make a value of that type.

Language/Haskell/GhcMod/ImportedFrom.hs
+ dflags <- GHC.getSessionDynFlags
+
+ setContext (map (IIDecl . simpleImportDecl . mkModuleName) (targetModuleName:importList))
+ `gcatch` (\(s :: SourceError) -> do gmLog GmDebug "qualifiedName"

This comment has been minimized.

@DanielG

DanielG Aug 27, 2016

Owner

I don't get why you thought duplicating basically the same function three times here is a good idea. I just removed all of them except the one for SomeException. show on SomeException actually prints the value of the message for the real exception so there really is no need to say which exception type it was. Even if that was desired you can always check what type is inside SomeException using Typeable, so no need for this duplication.

@DanielG

DanielG Aug 27, 2016

Owner

I don't get why you thought duplicating basically the same function three times here is a good idea. I just removed all of them except the one for SomeException. show on SomeException actually prints the value of the message for the real exception so there really is no need to say which exception type it was. Even if that was desired you can always check what type is inside SomeException using Typeable, so no need for this duplication.

Language/Haskell/GhcMod/ImportedFrom.hs
+refineRemoveHiding exports =
+ map (\e -> e { qualifiedExports = removeHiding e }) exports
+ where
+ removeHiding export = filter (`notElem` hiding') thisExports

This comment has been minimized.

@DanielG

DanielG Aug 27, 2016

Owner

thisExports :: [FullyQualifiedName] but hiding' :: [QualifiedName], those will never match AFAIU. The type system could've saved you here. Next time use it :p

@DanielG

DanielG Aug 27, 2016

Owner

thisExports :: [FullyQualifiedName] but hiding' :: [QualifiedName], those will never match AFAIU. The type system could've saved you here. Next time use it :p

This comment has been minimized.

@DanielG

DanielG Aug 27, 2016

Owner

Hmm nevermind, but I just realized the whole parsing below is completely pointless. Just keeping more information by making FullyQualifiedName a record is much easier.

@DanielG

DanielG Aug 27, 2016

Owner

Hmm nevermind, but I just realized the whole parsing below is completely pointless. Just keeping more information by making FullyQualifiedName a record is much easier.

@DanielG

This comment has been minimized.

Show comment
Hide comment
@DanielG

DanielG Aug 28, 2016

Owner

@carlohamalainen I think using ModIfaces directly might make this a lot more reliable. Have you looked into that already by any chance?

Owner

DanielG commented Aug 28, 2016

@carlohamalainen I think using ModIfaces directly might make this a lot more reliable. Have you looked into that already by any chance?

@DanielG

This comment has been minimized.

Show comment
Hide comment
@DanielG

DanielG Aug 28, 2016

Owner

Hmm nah that's a bad idea too.

I do wonder why you need to work with haddock's interfaces though? Doesn't GHC already have a superset of the information in there or am I missing something?

Owner

DanielG commented Aug 28, 2016

Hmm nah that's a bad idea too.

I do wonder why you need to work with haddock's interfaces though? Doesn't GHC already have a superset of the information in there or am I missing something?

@carlohamalainen

This comment has been minimized.

Show comment
Hide comment
@carlohamalainen

carlohamalainen Aug 28, 2016

Collaborator

Haddock provides instVisibleExports so I figured it best not to reimplement the wheel.

Collaborator

carlohamalainen commented Aug 28, 2016

Haddock provides instVisibleExports so I figured it best not to reimplement the wheel.

@DanielG

This comment has been minimized.

Show comment
Hide comment
@DanielG

DanielG Aug 30, 2016

Owner

What I really don't understand is why you have so many different ways to add/filter stuff out of the result set. Seems to me like the code could be much simpler. The thing that irks me most is how you use the pretty printer on things that will return syntax elements and not just identifiers (qualifiedNameAt) and then try parse the identifiers out of that soup again (refineRemoveHiding).

I mean maybe I'm just not understanding it but it does seem quite messy.

Owner

DanielG commented Aug 30, 2016

What I really don't understand is why you have so many different ways to add/filter stuff out of the result set. Seems to me like the code could be much simpler. The thing that irks me most is how you use the pretty printer on things that will return syntax elements and not just identifiers (qualifiedNameAt) and then try parse the identifiers out of that soup again (refineRemoveHiding).

I mean maybe I'm just not understanding it but it does seem quite messy.

@carlohamalainen

This comment has been minimized.

Show comment
Hide comment
@carlohamalainen

carlohamalainen Sep 4, 2016

Collaborator

When I first wrote ghc-imported-from I didn't foresee how complex it would become, parsing and filtering the soup of strings.

Is there a function in the GHC API to go from a file, line, col to a Name? Or from LHsBind Id?

Collaborator

carlohamalainen commented Sep 4, 2016

When I first wrote ghc-imported-from I didn't foresee how complex it would become, parsing and filtering the soup of strings.

Is there a function in the GHC API to go from a file, line, col to a Name? Or from LHsBind Id?

@DanielG

This comment has been minimized.

Show comment
Hide comment
@DanielG

DanielG Sep 4, 2016

Owner

No, most likely not but the names surely is in the AST so it is just a matter of traversing that (possibly using syb like we do for the type command).

Owner

DanielG commented Sep 4, 2016

No, most likely not but the names surely is in the AST so it is just a matter of traversing that (possibly using syb like we do for the type command).

@lierdakil

This comment has been minimized.

Show comment
Hide comment
@lierdakil

lierdakil Sep 4, 2016

Collaborator

Um... type Id = Var, instance NamedThing Var, so you can basically hsBindToName (LHsBind id) = getName id, right?

Collaborator

lierdakil commented Sep 4, 2016

Um... type Id = Var, instance NamedThing Var, so you can basically hsBindToName (LHsBind id) = getName id, right?

@DanielG

This comment has been minimized.

Show comment
Hide comment
@DanielG

DanielG Sep 4, 2016

Owner

Oh right, I missed the LHsBind Id bit.

Owner

DanielG commented Sep 4, 2016

Oh right, I missed the LHsBind Id bit.

carlohamalainen added a commit to carlohamalainen/ghcmod-vim that referenced this pull request Sep 24, 2016

@lierdakil

This comment has been minimized.

Show comment
Hide comment
@lierdakil

lierdakil Oct 22, 2016

Collaborator

So... let me get this straight. Sorry if I'm off the mark entirely.

Most code here is just dealing with getting module and package name of a named thing, yes? So couldn't we just getName on said named thing, which will get us a Name. Then we can apply nameModule_maybe to Name and get Module.

Now Module is a product type of UnitId (or PackageKey it's called in GHC-7.10) and ModuleName. Latter is self-explanatory, and we can get String representation with moduleNameString, and former we could use in lookupPackage to get PackageConfig, which will get us haddockHTMLs (which we can then happily filter by comparing to ModuleName with dots replaced by dashes), and package version, among other things.

Last step is a little more complicated with GHC-7.8, but it's essentially the same (just we'd have to dissect DynFlags a bit to get lookupPackage to work).

Am I missing something here?

Collaborator

lierdakil commented Oct 22, 2016

So... let me get this straight. Sorry if I'm off the mark entirely.

Most code here is just dealing with getting module and package name of a named thing, yes? So couldn't we just getName on said named thing, which will get us a Name. Then we can apply nameModule_maybe to Name and get Module.

Now Module is a product type of UnitId (or PackageKey it's called in GHC-7.10) and ModuleName. Latter is self-explanatory, and we can get String representation with moduleNameString, and former we could use in lookupPackage to get PackageConfig, which will get us haddockHTMLs (which we can then happily filter by comparing to ModuleName with dots replaced by dashes), and package version, among other things.

Last step is a little more complicated with GHC-7.8, but it's essentially the same (just we'd have to dissect DynFlags a bit to get lookupPackage to work).

Am I missing something here?

@carlohamalainen

This comment has been minimized.

Show comment
Hide comment
@carlohamalainen

carlohamalainen Oct 22, 2016

Collaborator

What does nameModule_maybe give for something like String? Will it tell me GHC.Base.String, which has no Haddock page, or Prelude, which is the sensible place to send the user since that is where the name is ultimately exported from?

Currently imported-from will say this:

base-4.8.2.0:GHC.Base.String Prelude https://hackage.haskell.org/package/base-4.8.2.0/docs/Prelude.html
Collaborator

carlohamalainen commented Oct 22, 2016

What does nameModule_maybe give for something like String? Will it tell me GHC.Base.String, which has no Haddock page, or Prelude, which is the sensible place to send the user since that is where the name is ultimately exported from?

Currently imported-from will say this:

base-4.8.2.0:GHC.Base.String Prelude https://hackage.haskell.org/package/base-4.8.2.0/docs/Prelude.html
@lierdakil

This comment has been minimized.

Show comment
Hide comment
@lierdakil

lierdakil Oct 22, 2016

Collaborator

You're right, nameModule will get an originating module, not an immediate module from which symbol was imported. Can't think of a way to rectify that from the top of my head, apart from stopping on Parser stage and working from there, but there might be one.

Collaborator

lierdakil commented Oct 22, 2016

You're right, nameModule will get an originating module, not an immediate module from which symbol was imported. Can't think of a way to rectify that from the top of my head, apart from stopping on Parser stage and working from there, but there might be one.

@lierdakil

This comment has been minimized.

Show comment
Hide comment
@lierdakil

lierdakil Oct 22, 2016

Collaborator

Okay, here's another idea.

We can limit ourselves to working with RenamedSource, which has list of used (located) Names and list of imports (almost verbatim from ParsedModule) separately. Then we can iterate over imports: for each ModuleName, we can get Module with findImportedModule (since source is renamed/typechecked, all imports should be in current HscEnv), and we can get renamed module exports with getModuleInfo and modInfoExports (or possibly we could use modInfoIsExportedName, since we don't actually need all the exports). Then just filter until we find module that imports symbol we're interested in. Hiding etc can be used as a tiebreaker.

Compared to current approach, working with RenamedSource allows to work with fully-qualified Names directly, so most of the awkwardness should be gone.

This is a bit more work then my previous idea, but could probably be condensed to something like 100-200 LOC instead of 900.

Collaborator

lierdakil commented Oct 22, 2016

Okay, here's another idea.

We can limit ourselves to working with RenamedSource, which has list of used (located) Names and list of imports (almost verbatim from ParsedModule) separately. Then we can iterate over imports: for each ModuleName, we can get Module with findImportedModule (since source is renamed/typechecked, all imports should be in current HscEnv), and we can get renamed module exports with getModuleInfo and modInfoExports (or possibly we could use modInfoIsExportedName, since we don't actually need all the exports). Then just filter until we find module that imports symbol we're interested in. Hiding etc can be used as a tiebreaker.

Compared to current approach, working with RenamedSource allows to work with fully-qualified Names directly, so most of the awkwardness should be gone.

This is a bit more work then my previous idea, but could probably be condensed to something like 100-200 LOC instead of 900.

@lierdakil

This comment has been minimized.

Show comment
Hide comment
@lierdakil

lierdakil Oct 22, 2016

Collaborator

P.S. findModule instead of findImportedModule would probably be better in this case, since we're pretty sure that it wouldn't throw, and result doesn't need unwrapping.

Collaborator

lierdakil commented Oct 22, 2016

P.S. findModule instead of findImportedModule would probably be better in this case, since we're pretty sure that it wouldn't throw, and result doesn't need unwrapping.

@lierdakil

This comment has been minimized.

Show comment
Hide comment
@lierdakil

lierdakil Oct 22, 2016

Collaborator

Here's an extremely rough sketch of what I was thinking: 3f282a8

Of course, it's not good code, there are a few irrefutable patterns that might be refuted, it doesn't do exactly the same, findSpanName could be more specific, and performance is probably horrible due to use of list operations like nub and \\. But should be enough to get my point across.

Collaborator

lierdakil commented Oct 22, 2016

Here's an extremely rough sketch of what I was thinking: 3f282a8

Of course, it's not good code, there are a few irrefutable patterns that might be refuted, it doesn't do exactly the same, findSpanName could be more specific, and performance is probably horrible due to use of list operations like nub and \\. But should be enough to get my point across.

@carlohamalainen

This comment has been minimized.

Show comment
Hide comment
@carlohamalainen

carlohamalainen Oct 23, 2016

Collaborator

Looks good. Any reason to sort on the spans? I mean this line:

sortBy (cmp `on` fst) $ findSpanName decls (lineNr, colNr)

The order of the output isn't consistent:

Since Data.Maybe is imported, the correct answer is second result:

$ ghc-mod imported-from test/data/imported-from/ImportedFrom01.hs 12 7 Just
Warning: ghc-mod.cabal: Ignoring unknown section type: custom-setup
base-4.8.2.0:GHC.Base.Just Prelude,Data.Maybe /opt/ghc/7.10.3/share/doc/ghc/html/libraries/base-4.8.2.0/Prelude.html /opt/ghc/7.10.3/share/doc/ghc/html/libraries/base-4.8.2.0/Data-Maybe.html

Since DL.length is qualified, the correct answer is the second/third result:

$ ghc-mod imported-from test/data/imported-from/ImportedFrom01.hs 23 5 DL.length
base-4.8.2.0:Data.Foldable.length Prelude,Data.List,Data.List /opt/ghc/7.10.3/share/doc/ghc/html/libraries/base-4.8.2.0/Prelude.html /opt/ghc/7.10.3/share/doc/ghc/html/libraries/base-4.8.2.0/Data-List.html /opt/ghc/7.10.3/share/doc/ghc/html/libraries/base-4.8.2.0/Data-List.html

In this case, buildMatrix is not in Numeric.Container, it is in Data.Packed.Matrix, the first result:

$ ghc-mod imported-from Mat.hs 13 8 buildMatrix
hmatrix-0.16.1.5:Data.Packed.Matrix.buildMatrix Data.Packed.Matrix,Numeric.Container /home/carlo/matrix/.cabal-sandbox/share/doc/x86_64-linux-ghc-7.10.3/hmatrix-0.16.1.5/html/Data-Packed-Matrix.html /home/carlo/matrix/.cabal-sandbox/share/doc/x86_64-linux-ghc-7.10.3/hmatrix-0.16.1.5/html/Numeric-Container.html

(source code for Mat.hs at https://carlo-hamalainen.net/stuff/matrix/)

Collaborator

carlohamalainen commented on 3f282a8 Oct 23, 2016

Looks good. Any reason to sort on the spans? I mean this line:

sortBy (cmp `on` fst) $ findSpanName decls (lineNr, colNr)

The order of the output isn't consistent:

Since Data.Maybe is imported, the correct answer is second result:

$ ghc-mod imported-from test/data/imported-from/ImportedFrom01.hs 12 7 Just
Warning: ghc-mod.cabal: Ignoring unknown section type: custom-setup
base-4.8.2.0:GHC.Base.Just Prelude,Data.Maybe /opt/ghc/7.10.3/share/doc/ghc/html/libraries/base-4.8.2.0/Prelude.html /opt/ghc/7.10.3/share/doc/ghc/html/libraries/base-4.8.2.0/Data-Maybe.html

Since DL.length is qualified, the correct answer is the second/third result:

$ ghc-mod imported-from test/data/imported-from/ImportedFrom01.hs 23 5 DL.length
base-4.8.2.0:Data.Foldable.length Prelude,Data.List,Data.List /opt/ghc/7.10.3/share/doc/ghc/html/libraries/base-4.8.2.0/Prelude.html /opt/ghc/7.10.3/share/doc/ghc/html/libraries/base-4.8.2.0/Data-List.html /opt/ghc/7.10.3/share/doc/ghc/html/libraries/base-4.8.2.0/Data-List.html

In this case, buildMatrix is not in Numeric.Container, it is in Data.Packed.Matrix, the first result:

$ ghc-mod imported-from Mat.hs 13 8 buildMatrix
hmatrix-0.16.1.5:Data.Packed.Matrix.buildMatrix Data.Packed.Matrix,Numeric.Container /home/carlo/matrix/.cabal-sandbox/share/doc/x86_64-linux-ghc-7.10.3/hmatrix-0.16.1.5/html/Data-Packed-Matrix.html /home/carlo/matrix/.cabal-sandbox/share/doc/x86_64-linux-ghc-7.10.3/hmatrix-0.16.1.5/html/Numeric-Container.html

(source code for Mat.hs at https://carlo-hamalainen.net/stuff/matrix/)

This comment has been minimized.

Show comment
Hide comment
@lierdakil

lierdakil Oct 23, 2016

Collaborator

Any reason to sort on the spans?

Not in this implementation, which filters results based on Expression symbol. However, if it's omitted (that possibility is not implemented here), we can make a very good guess about what that symbol is based on first element of this sorted list, since it's the smallest SrcSpan that contains (lineNr, colNr).

Since Data.Maybe is imported, the correct answer is second result

Actually, both Prelude and Data.Maybe reexport the same symbol, so technically both are equally valid. Output order depends on order of imports (implicit prelude import is always the first). I'm not too sure on what the tiebreaker rule would be in such cases.

Since DL.length is qualified, the correct answer is the second/third result

Again, same thing generally speaking. Can filter imports based on qualification though. Should note that occName isn't qualified. We could of course grab qualified names from ParsedSource.

In this case, buildMatrix is not in Numeric.Container, it is in Data.Packed.Matrix, the first result

Okay, so here we run into a bit of a problem. Numeric.Container reexports Data.Packed, which reexports Data.Packed.Matrix, so technically both results are valid. However, documentation is only in the latter. To further complicate things, neither of those actually defines buildMatrix, which is instead defined in Data.Packed.Internal.Matrix (or something to the same effect). Here's where haddock interface files would come in handy, with instVisibleExports -- we can get package's haddock interface files from Package.

Collaborator

lierdakil replied Oct 23, 2016

Any reason to sort on the spans?

Not in this implementation, which filters results based on Expression symbol. However, if it's omitted (that possibility is not implemented here), we can make a very good guess about what that symbol is based on first element of this sorted list, since it's the smallest SrcSpan that contains (lineNr, colNr).

Since Data.Maybe is imported, the correct answer is second result

Actually, both Prelude and Data.Maybe reexport the same symbol, so technically both are equally valid. Output order depends on order of imports (implicit prelude import is always the first). I'm not too sure on what the tiebreaker rule would be in such cases.

Since DL.length is qualified, the correct answer is the second/third result

Again, same thing generally speaking. Can filter imports based on qualification though. Should note that occName isn't qualified. We could of course grab qualified names from ParsedSource.

In this case, buildMatrix is not in Numeric.Container, it is in Data.Packed.Matrix, the first result

Okay, so here we run into a bit of a problem. Numeric.Container reexports Data.Packed, which reexports Data.Packed.Matrix, so technically both results are valid. However, documentation is only in the latter. To further complicate things, neither of those actually defines buildMatrix, which is instead defined in Data.Packed.Internal.Matrix (or something to the same effect). Here's where haddock interface files would come in handy, with instVisibleExports -- we can get package's haddock interface files from Package.

lierdakil added some commits Oct 23, 2016

[imported-from] Allow outputting several names
Case when this is relevant: consider a query for source point like this:
something   other
          ^--- queryed here
Which one do we choose, former or latter? I would argue we choose both
and let the client decide.
[imported-from] Fix some things with `showOutput`
Apparently, ghc 7.8 can be picky when to give you a package version.
It can return just empty version for wired-in packages like base, etc.
So I suppose we have to guess those from haddock file path if we can...
@lierdakil

This comment has been minimized.

Show comment
Hide comment
@lierdakil

lierdakil Oct 25, 2016

Collaborator

I think at this point this is "good enough". I could probably refine this ad infinum given the chance, but I think I have to stop at some point, and this particular point seems as good as any.

@DanielG, if you could review this, that would be cool.

Collaborator

lierdakil commented Oct 25, 2016

I think at this point this is "good enough". I could probably refine this ad infinum given the chance, but I think I have to stop at some point, and this particular point seems as good as any.

@DanielG, if you could review this, that would be cool.

@DanielG

This comment has been minimized.

Show comment
Hide comment
@DanielG

DanielG Oct 26, 2016

Owner

Looks good, though we had a mode to just print the package+module name instead of the full haddock path (see comments following #810 (comment)) . @lierdakil did you remove that?

Also we seem to have reverted to using error to report user facing errors which we fixed before, see fa1417a for example.

Owner

DanielG commented Oct 26, 2016

Looks good, though we had a mode to just print the package+module name instead of the full haddock path (see comments following #810 (comment)) . @lierdakil did you remove that?

Also we seem to have reverted to using error to report user facing errors which we fixed before, see fa1417a for example.

@lierdakil

This comment has been minimized.

Show comment
Hide comment
@lierdakil

lierdakil Oct 26, 2016

Collaborator

Looks good, though we had a mode to just print the package+module name instead of the full haddock path (see comments following #810 (comment)) . @lierdakil did you remove that?

Yep, missed that bit. I basically reimplemented the whole deal from scratch while stealing bits and pieces from @carlohamalainen's implementation when appropriate, so I was bound to miss something.

Although I should point out that while hackage url can be, in general, inferred from package id, module and symbol, local haddock path can not, and current implementation prefers the latter if it's available. So there's that. I think we can solve this problem in another way though, see below.

Also we seem to have reverted to using error to report user facing errors which we fixed before, see fa1417a for example.

Reason being, those errors never leave the scope of the function, so my reasoning was that why define exceptions that are local to the module. Now that I think about it, I should probably make it throw GME exceptions and handle those downstream, since it might be more useful to library users. Sorry, didn't think about that -- was OD'ing on caffeine at the time, so I kinda rushed a bit.

Also while on the topic of library users. I've been bitching about library interface for a while (#696), and now seems as good a time as any to start imposing at least some structure on the returned values. So what do you think about returning [ImportedFromResult] instead of String? We can make an instance of TextOutput ImportedFromResult (and also TextOutput a => TextOutput [a]) to convert that to String in ghc-mod binary. Also we can dump the whole batch of information out, including package name and hackage url here, without having to pass a ton of flags to the function itself (we could instead add those into TextOutput instances -- e.g. by defining an instance-specific output config)
Thoughts?

Collaborator

lierdakil commented Oct 26, 2016

Looks good, though we had a mode to just print the package+module name instead of the full haddock path (see comments following #810 (comment)) . @lierdakil did you remove that?

Yep, missed that bit. I basically reimplemented the whole deal from scratch while stealing bits and pieces from @carlohamalainen's implementation when appropriate, so I was bound to miss something.

Although I should point out that while hackage url can be, in general, inferred from package id, module and symbol, local haddock path can not, and current implementation prefers the latter if it's available. So there's that. I think we can solve this problem in another way though, see below.

Also we seem to have reverted to using error to report user facing errors which we fixed before, see fa1417a for example.

Reason being, those errors never leave the scope of the function, so my reasoning was that why define exceptions that are local to the module. Now that I think about it, I should probably make it throw GME exceptions and handle those downstream, since it might be more useful to library users. Sorry, didn't think about that -- was OD'ing on caffeine at the time, so I kinda rushed a bit.

Also while on the topic of library users. I've been bitching about library interface for a while (#696), and now seems as good a time as any to start imposing at least some structure on the returned values. So what do you think about returning [ImportedFromResult] instead of String? We can make an instance of TextOutput ImportedFromResult (and also TextOutput a => TextOutput [a]) to convert that to String in ghc-mod binary. Also we can dump the whole batch of information out, including package name and hackage url here, without having to pass a ton of flags to the function itself (we could instead add those into TextOutput instances -- e.g. by defining an instance-specific output config)
Thoughts?

@DanielG

This comment has been minimized.

Show comment
Hide comment
@DanielG

DanielG Oct 26, 2016

Owner

Returning proper datatypes instead of strings sounds lovely actually I don't see why we shouldn't.

Owner

DanielG commented Oct 26, 2016

Returning proper datatypes instead of strings sounds lovely actually I don't see why we shouldn't.

carlohamalainen added a commit to carlohamalainen/ghcmod-vim that referenced this pull request Aug 14, 2017

@carlohamalainen

This comment has been minimized.

Show comment
Hide comment
@carlohamalainen

carlohamalainen Aug 14, 2017

Collaborator

I updated this PR against the master branch but I'm not seeing any status from travis - is there something else that I have to do?

Collaborator

carlohamalainen commented Aug 14, 2017

I updated this PR against the master branch but I'm not seeing any status from travis - is there something else that I have to do?

@DanielG

This comment has been minimized.

Show comment
Hide comment
@DanielG

DanielG Aug 14, 2017

Owner

We've stopped using travis a while ago. The builds are now on Gitlab(CI) but it looks like the repo mirroring has broken, I'm looking into that. If you have a gitlab account I can add you there so you can push this branch to the other repo as well, which will trigger a build.

FYI you can actually configure git to push to multiple repos at once when you push to a remote, see https://gist.github.com/bjmiller121/f93cd974ff709d2b968f

Owner

DanielG commented Aug 14, 2017

We've stopped using travis a while ago. The builds are now on Gitlab(CI) but it looks like the repo mirroring has broken, I'm looking into that. If you have a gitlab account I can add you there so you can push this branch to the other repo as well, which will trigger a build.

FYI you can actually configure git to push to multiple repos at once when you push to a remote, see https://gist.github.com/bjmiller121/f93cd974ff709d2b968f

@carlohamalainen

This comment has been minimized.

Show comment
Hide comment
Collaborator

carlohamalainen commented Aug 14, 2017

Just made an account: https://gitlab.com/carlohamalainen

@DanielG

This comment has been minimized.

Show comment
Hide comment
@DanielG

DanielG Aug 15, 2017

Owner

Done

Owner

DanielG commented Aug 15, 2017

Done

@DanielG DanielG modified the milestones: v5.9.0.0, Whenever Sep 14, 2017

@DanielG

This comment has been minimized.

Show comment
Hide comment
@DanielG

DanielG Sep 14, 2017

Owner

@carlohamalainen are you still working on this? Do you need help with the new CI setup or something?

Owner

DanielG commented Sep 14, 2017

@carlohamalainen are you still working on this? Do you need help with the new CI setup or something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment