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

Do not show internal hole names #852

Merged
merged 6 commits into from
Oct 10, 2020

Conversation

serras
Copy link
Contributor

@serras serras commented Oct 5, 2020

Fixes #847

This PR solves both problems:

  • It contains a regular expression to catch internal names for holes /_[^_]*_.....?/ and rewrite them to _,
  • It ensures that "no location info" is not shown by filtering out "unhelpful locations"

@serras
Copy link
Contributor Author

serras commented Oct 5, 2020

If this PR gets accepted, may I ask to add the corresponding tag to count it towards my Hacktoberfest goal?

Maintainers of the repository can add the "hacktoberfest" topic to their repository if they wish to participate. Alternatively, an individual PR can be opted-in with a maintainer adding the "hacktoberfest-accepted" label to the PR.

@wz1000
Copy link
Collaborator

wz1000 commented Oct 6, 2020

I'm not sure this is the best way to go about this. id __foo_bar_baz = __foo_bar_baz is a perfectly valid definition.

GHC already pretty prints these nicely in error messages. There must be some way to twiddle DynFlags to achieve the same effect.

@pepeiborra
Copy link
Collaborator

If this PR gets accepted, may I ask to add the corresponding tag to count it towards my Hacktoberfest goal?

Maintainers of the repository can add the "hacktoberfest" topic to their repository if they wish to participate. Alternatively, an individual PR can be opted-in with a maintainer adding the "hacktoberfest-accepted" label to the PR.

This is done, I have added the topic

@alanz
Copy link
Collaborator

alanz commented Oct 6, 2020

I think the -dsuppress-uniques flag does this. Opt_SuppressUniques.

@serras
Copy link
Contributor Author

serras commented Oct 6, 2020

@wz1000 you are completely right. I found another way to do this, but first I want to check @alanz's suggestion.

@serras serras force-pushed the no-internal-names-hole branch from d3e27df to de7092d Compare October 8, 2020 11:58
@serras
Copy link
Contributor Author

serras commented Oct 9, 2020

I am very puzzled, the tests seem to break randomly on this part:

f = (:|):                                                                                         FAIL (0.82s)
  test/exe/Main.hs:3520:
  Found no matching actions for "import Data.List.NonEmpty (NonEmpty((:|)))" in []
f :: Natural:                                                                                     FAIL (0.80s)
  test/exe/Main.hs:3520:
  Found no matching actions for "import Numeric.Natural (Natural)" in []

However, in the previous push 8.10 broke, and now 8.8 breaks.

@pepeiborra @wz1000 @jneira (maybe somebody else) could you help me debugging this? Thanks in advance :)

@serras
Copy link
Contributor Author

serras commented Oct 9, 2020

By the way, @alanz's approach of using SuppressUniques did the job, and now holes are shown with their original names :)

@pepeiborra
Copy link
Collaborator

I am very puzzled, the tests seem to break randomly on this part:

f = (:|):                                                                                         FAIL (0.82s)
  test/exe/Main.hs:3520:
  Found no matching actions for "import Data.List.NonEmpty (NonEmpty((:|)))" in []
f :: Natural:                                                                                     FAIL (0.80s)
  test/exe/Main.hs:3520:
  Found no matching actions for "import Numeric.Natural (Natural)" in []

However, in the previous push 8.10 broke, and now 8.8 breaks.

@pepeiborra @wz1000 @jneira (maybe somebody else) could you help me debugging this? Thanks in advance :)

I think this happens because you added a hole to the GotoHover module which is used by these tests. GHC is quite slow at providing fill hole suggestions, and these tests are probably not waiting for diagnostics before asking for code actions (should they?). Either fix the tests to wait for diagnostics, or put the hole somewhere else.

@serras
Copy link
Contributor Author

serras commented Oct 9, 2020

I managed to make the import suggestions work by removing GotoHover from the cradle. Although there is a commit adding more time to those tests, it has been reverted later on.

test/exe/Main.hs Outdated
@@ -1121,7 +1121,7 @@ suggestImportTests = testGroup "suggest import actions"
void (skipManyTill anyMessage message :: Session WorkDoneProgressEndNotification)
_diags <- waitForDiagnostics
-- there isn't a good way to wait until the whole project is checked atm
when waitForCheckProject $ liftIO $ sleep 0.5
when waitForCheckProject $ liftIO $ sleep 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Times in tests make me very nervous. Shouldn't you just wait for the diagnostics to get published?

Copy link
Collaborator

Choose a reason for hiding this comment

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

these tests are checking for import suggestions, which rely either on the full project being checked (for local imports) or the transitive build-depends being loaded (for package imports). Both actions happen asynchronously and ghcide does not send any notification that can be used to wait currently. Would be easy to fix but certainly doesn't belong in this PR

@pepeiborra pepeiborra merged commit bfe2563 into haskell:master Oct 10, 2020
@pepeiborra
Copy link
Collaborator

Thanks!

pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Do not show internal hole names

* Better way to print holes as _

* Use suggestion by @alanz

* Remove unneeded import

* Give more time to suggestion tests

* Do not import GotoHover for testing suggestions
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Do not show internal hole names

* Better way to print holes as _

* Use suggestion by @alanz

* Remove unneeded import

* Give more time to suggestion tests

* Do not import GotoHover for testing suggestions
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Do not show internal hole names

* Better way to print holes as _

* Use suggestion by @alanz

* Remove unneeded import

* Give more time to suggestion tests

* Do not import GotoHover for testing suggestions
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.

Internal names displayed in hover for a typed hole
4 participants