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

Tests for issue 310 (misleading hover on inner signature) #311

Merged
merged 3 commits into from Jan 8, 2020

Conversation

jacg
Copy link
Contributor

@jacg jacg commented Jan 7, 2020

Tests for #310.

The most important pair of tests here is the "inner signature" pair. The others
serve mainly to document, compare and contrast what is happening in related
situations.

In summary, hover and gotoDef

  • on inner signatures: give type and location information for the outer
    definition; this is misleading,

  • on outer signatures: give no information at all,

  • on inner definitions: give correct information for the inner definition,

  • on outer definitions: give correct information for the outer definition.

Should hover and gotoDef do anything at all for signatures? or is the current
behaviour for outer signatures (doing nothing at all) what we want?

The most important pair of tests here is the "inner signature" pair. The others
serve mainly to document, compare and contrast what is happening in related
situations.

In summary, hover and gotoDef

+ on inner signatures: give type and location information for the outer
  definition; this is misleading,

+ on outer signatures: give no information at all,

+ on inner definitions: give correct information for the inner definition,

+ on outer definitions: give correct information for the outer definition.

Should hover and gotoDef do anything at all for signatures? or is the current
behaviour for outer signatures (doing nothing at all) what we want?
Copy link
Contributor

@aherrmann-da aherrmann-da left a comment

Choose a reason for hiding this comment

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

Thanks for adding these tests.

I'd agree that hover/gotoDef on the signature should point to their corresponding definition (i.e. outer->outer and inner->inner).

@jacg
Copy link
Contributor Author

jacg commented Jan 7, 2020

I'd agree that hover/gotoDef on the signature should point to their corresponding definition (i.e. outer->outer and inner->inner).

Which leaves one small detail: what exactly do we mean by "pont to their definition"? Should it point to

  1. The signature (which is guaranteed to exist, given that we are considering hovering on a signature)
  2. The first clause in the definition

?

@aherrmann-da
Copy link
Contributor

Which leaves one small detail: what exactly do we mean by "pont to their definition"? Should it point to

  1. The signature (which is guaranteed to exist, given that we are considering hovering on a signature)
  2. The first clause in the definition

?

Right, thanks for pointing that out. In other instances "gotoDef" does (2). In a snippet like the following (2) also seems more useful.

a, b :: Int
a = 1
b = 2

So, to me (2) seems like the better option.

@jacg
Copy link
Contributor Author

jacg commented Jan 8, 2020

(2) seems like the better option.

Agreed. I'll adjust the tests to reflect this choice.

@jacg
Copy link
Contributor Author

jacg commented Jan 8, 2020

I'll adjust the tests to reflect this choice.

Done.

I'm not sure whether it is worth keeping the tests which compare the (working) definition behaviour to the (broken) signature behaviour, so I have added a separate commit which removes the definition tests: we can keep or revert this commit depending on how we feel about those tests.

@cocreature cocreature merged commit 5f4384e into haskell:master Jan 8, 2020
@cocreature
Copy link
Collaborator

Thanks! Happy to remove the tests since as you pointed out they are somewhat redundant.

@jacg jacg deleted the inner-type-sig-test branch January 9, 2020 07:48
pepeiborra pushed a commit to pepeiborra/ghcide that referenced this pull request Feb 1, 2020
* Tests for issue 310 (misleading hover on inner signature)

The most important pair of tests here is the "inner signature" pair. The others
serve mainly to document, compare and contrast what is happening in related
situations.

In summary, hover and gotoDef

+ on inner signatures: give type and location information for the outer
  definition; this is misleading,

+ on outer signatures: give no information at all,

+ on inner definitions: give correct information for the inner definition,

+ on outer definitions: give correct information for the outer definition.

Should hover and gotoDef do anything at all for signatures? or is the current
behaviour for outer signatures (doing nothing at all) what we want?

* Require signature hover/gotoDef to point to first clause of definition

* Remove perhaps superfluous tests for definitions
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
…cide#311)

* Tests for issue 310 (misleading hover on inner signature)

The most important pair of tests here is the "inner signature" pair. The others
serve mainly to document, compare and contrast what is happening in related
situations.

In summary, hover and gotoDef

+ on inner signatures: give type and location information for the outer
  definition; this is misleading,

+ on outer signatures: give no information at all,

+ on inner definitions: give correct information for the inner definition,

+ on outer definitions: give correct information for the outer definition.

Should hover and gotoDef do anything at all for signatures? or is the current
behaviour for outer signatures (doing nothing at all) what we want?

* Require signature hover/gotoDef to point to first clause of definition

* Remove perhaps superfluous tests for definitions
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
…cide#311)

* Tests for issue 310 (misleading hover on inner signature)

The most important pair of tests here is the "inner signature" pair. The others
serve mainly to document, compare and contrast what is happening in related
situations.

In summary, hover and gotoDef

+ on inner signatures: give type and location information for the outer
  definition; this is misleading,

+ on outer signatures: give no information at all,

+ on inner definitions: give correct information for the inner definition,

+ on outer definitions: give correct information for the outer definition.

Should hover and gotoDef do anything at all for signatures? or is the current
behaviour for outer signatures (doing nothing at all) what we want?

* Require signature hover/gotoDef to point to first clause of definition

* Remove perhaps superfluous tests for definitions
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
…cide#311)

* Tests for issue 310 (misleading hover on inner signature)

The most important pair of tests here is the "inner signature" pair. The others
serve mainly to document, compare and contrast what is happening in related
situations.

In summary, hover and gotoDef

+ on inner signatures: give type and location information for the outer
  definition; this is misleading,

+ on outer signatures: give no information at all,

+ on inner definitions: give correct information for the inner definition,

+ on outer definitions: give correct information for the outer definition.

Should hover and gotoDef do anything at all for signatures? or is the current
behaviour for outer signatures (doing nothing at all) what we want?

* Require signature hover/gotoDef to point to first clause of definition

* Remove perhaps superfluous tests for definitions
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.

None yet

3 participants