Skip to content
This repository has been archived by the owner on Jan 2, 2021. It is now read-only.

Fix documentation (or source) link when html file is less specific than module #766

Merged
merged 5 commits into from
Sep 20, 2020

Conversation

DunetsNM
Copy link
Contributor

@DunetsNM DunetsNM commented Sep 5, 2020

Below is an example when documentation is available but wasn't located because file name is less specific than module. This PR fixes it

Documentation:
Note the difference in doc file name and module name.

image

Source:
In this case both src and module name are the same.

image

Other notes:

  • Also all directories attempted (not sure if it can possibly be more than one directory but original code is trying only first anyway)
  • Just in case I'm on Windows (10)
  • I'm experiencing more troubles with docs/sources on Windows (docs for most libs reported to be at $topdir/../ which doesn't work on Windows) but let's tackle one thing at a time.
  • using stack

@DunetsNM DunetsNM marked this pull request as ready for review September 5, 2020 10:38
@pepeiborra
Copy link
Collaborator

Thanks for your contribution! I note that this feature lacks a test suite, and therefore it's very hard to tell whether the changes you propose break anything, in any of the supported platforms and build tools.

Would you be able to contribute such a test suite? Ideally it would cover projects using both cabal and stack, and our CI system will take care of running both in Windows and LInux

@pepeiborra pepeiborra requested a review from lukel97 September 6, 2020 09:20
@wz1000
Copy link
Collaborator

wz1000 commented Sep 6, 2020

This approach is not correct, strictly speaking. Instead of stripping out the last part of the module name (which has no relation with the module that actually exports the documentation for the symbol), a better idea would be to look in the tcg_rdr_env to check which import actually brought the name into scope and then looking up the haddocks for that module.

@lukel97
Copy link
Collaborator

lukel97 commented Sep 7, 2020

Should be fine now as a plaster, but it would be nice to do it the proper way at some point as well as what @wz1000 said
@pepeiborra for the tests I think there's some functional tests to make sure that the Documentation/Source things are generated but they don't actually check for the correct URL:

ghcide/test/exe/Main.hs

Lines 2171 to 2177 in 0d7cae9

#if MIN_GHC_API_VERSION(8,6,0)
imported = Position 56 13 ; importedSig = getDocUri "Foo.hs" >>= \foo -> return [ExpectHoverText ["foo", "Foo", "Haddock"], mkL foo 5 0 5 3]
reexported = Position 55 14 ; reexportedSig = getDocUri "Bar.hs" >>= \bar -> return [ExpectHoverText ["Bar", "Bar", "Haddock"], mkL bar 3 0 3 14]
#else
imported = Position 56 13 ; importedSig = getDocUri "Foo.hs" >>= \foo -> return [ExpectHoverText ["foo", "Foo"], mkL foo 5 0 5 3]
reexported = Position 55 14 ; reexportedSig = getDocUri "Bar.hs" >>= \bar -> return [ExpectHoverText ["Bar", "Bar"], mkL bar 3 0 3 14]
#endif

@pepeiborra
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pepeiborra pepeiborra merged commit 55e3810 into haskell:master Sep 20, 2020
@DunetsNM DunetsNM deleted the feature/clever-doc-lookup branch September 20, 2020 10:19
@DunetsNM
Copy link
Contributor Author

DunetsNM commented Oct 2, 2020

This approach is not correct, strictly speaking. Instead of stripping out the last part of the module name (which has no relation with the module that actually exports the documentation for the symbol), a better idea would be to look in the tcg_rdr_env to check which import actually brought the name into scope and then looking up the haddocks for that module.

@wz1000 This is probably neither the ultimate solution. The "NormalizedFilePath" from the original screenshot is imported from local module "Development.IDE.Types.Location" - not module from the package for which haddock file exists (confirmed it by looking up the import decl in GlobalRdrEnv : mi_globals on the containing ModIface, then located the GlobalRdrElt by Name).
Even if we go down recursively to the package module it also doesn't guarantee that imported module from the package is the one that has corresponding Haddock file (I don't think so).

One counter-example is import Control.Lens which brings a lot of stuff into scope, but corresponding haddock file (doc/lens-4.17.1/Control-Lens.html) is just a short Table of Contents to sub-module htmls.

Overall, trying to replicate Haddock's logic feels wrong, it might be better to start from the opposite end e.g. parse doc-index.json to pinpoint exact location and anchor (unless there's any simpler api to parse Haddock index available - I don't know any)

pepeiborra pushed a commit to pepeiborra/ghcide that referenced this pull request Oct 5, 2020
…an module (haskell#766)

* show doc/source link when html file name is less specific than module name

* try most qualified file names first, both dash and dot delimited

* small cleanup

* make hlint happy

* hlint again
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
…an module (haskell/ghcide#766)

* show doc/source link when html file name is less specific than module name

* try most qualified file names first, both dash and dot delimited

* small cleanup

* make hlint happy

* hlint again
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
…an module (haskell/ghcide#766)

* show doc/source link when html file name is less specific than module name

* try most qualified file names first, both dash and dot delimited

* small cleanup

* make hlint happy

* hlint again
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
…an module (haskell/ghcide#766)

* show doc/source link when html file name is less specific than module name

* try most qualified file names first, both dash and dot delimited

* small cleanup

* make hlint happy

* hlint again
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants