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

Fix docs tooltip for base libraries on Windows #814

Merged
merged 5 commits into from
Sep 20, 2020

Conversation

DunetsNM
Copy link
Contributor

This fixes #769 , see the issue thread for details

CHANGELOG.md Outdated
@@ -1,4 +1,26 @@
### unreleased
### 0.4.0 (2020-09-15)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this should end up in this diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I accidentally branched off latest / 0.4.0 prerelease , guess this PR won't get into 0.4 anyway so can just wait until Pepe's commit get into master

Copy link
Collaborator

@ndmitchell ndmitchell left a comment

Choose a reason for hiding this comment

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

Is there any way to write a test for this code? I imagine if we did a doesFileExist on the resulting path in the tooltip, before this diff it wouldn't have worked, after it will? I'm sensitive that this whole area feels a bit easy to break in future.

-- canonicalize located html to remove /../ indirection which can break some clients
-- (vscode on Windows at least)
htmls' <- traverse canonicalizePath htmls
return $ listToMaybe htmls'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code applies doesFileExist/canonicalizePath to a sequence of items, then only takes the first. Yuk. That means we are paying for more IO operations that we need. Can we change the filterM to be a findM, then we have a Maybe FIlePath, you can still traverse it to canonicalise, but now we only canonicalize one element, and can skip the listToMaybe.

(I appreciate this wasn't your code to begin with, but a good thing to fix)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did only the easy change so far, will look into testability a bit later
Thanks for the hint re findM, didn't know it (will take it as reminder to use Hoogle more often). I also had a thought about potentially wasteful IO on file exists/canonicalize , my naive intuition was that laziness will take care of it (which is not the case as it turned out)

@ndmitchell
Copy link
Collaborator

And thanks for the diff - its super helpful - I've encountered this issue on Windows too and it was a pain!

@jneira
Copy link
Member

jneira commented Sep 17, 2020

very happy to see contributions for Windows, many thanks @DunetsNM

@pepeiborra pepeiborra merged commit 20ce9d3 into haskell:master Sep 20, 2020
@DunetsNM DunetsNM deleted the fix-docs-on-windows branch September 20, 2020 10:15
pepeiborra added a commit to pepeiborra/ghcide that referenced this pull request Oct 5, 2020
* Prepare for release 0.4.0

* lookup haddock dir via haddockInterfaces

* Fix broken base libraries documentation on Windows

* use findM to get just first existing file

Co-authored-by: Pepe Iborra <pepeiborra@gmail.com>
pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Prepare for release 0.4.0

* lookup haddock dir via haddockInterfaces

* Fix broken base libraries documentation on Windows

* use findM to get just first existing file

Co-authored-by: Pepe Iborra <pepeiborra@gmail.com>
pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Prepare for release 0.4.0

* lookup haddock dir via haddockInterfaces

* Fix broken base libraries documentation on Windows

* use findM to get just first existing file

Co-authored-by: Pepe Iborra <pepeiborra@gmail.com>
pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Prepare for release 0.4.0

* lookup haddock dir via haddockInterfaces

* Fix broken base libraries documentation on Windows

* use findM to get just first existing file

Co-authored-by: Pepe Iborra <pepeiborra@gmail.com>
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.

(Windows, Stack) Documentation / Source links don't appear for base libraries, only for stackage
4 participants