Skip to content
This repository was archived by the owner on Oct 7, 2020. It is now read-only.

Add ReqTypeDefinition to reactor #1107

Merged
merged 17 commits into from
Apr 8, 2019
Merged

Add ReqTypeDefinition to reactor #1107

merged 17 commits into from
Apr 8, 2019

Conversation

fendor
Copy link
Collaborator

@fendor fendor commented Feb 26, 2019 •
edited
Loading

Closes #1010

Missing:

  • Unit-tests
  • Integration tests with lsp-test , see #31
  • Validating it actually does something

Works for variables and sum type values.
Implement tests for data, newtype and type.
For type defs, the original type definition will be found.
If the data type definition is not in scope, empty result will be sent.

@fendor fendor mentioned this pull request Feb 26, 2019
Merged
@alanz alanz modified the milestones: 2019-02, 2019-03 Mar 2, 2019
@fendor fendor force-pushed the type-definition-request branch 3 times, most recently from 91326a2 to 0fb0bdd Compare March 29, 2019 14:59
@fendor fendor requested review from alanz, lukel97 and wz1000 March 29, 2019 15:02
@fendor fendor force-pushed the type-definition-request branch from 0fb0bdd to bafdc10 Compare March 29, 2019 15:07
lorenzo
lorenzo reviewed Mar 30, 2019
View reviewed changes
lukel97
lukel97 reviewed Mar 31, 2019
View reviewed changes
Copy link
Collaborator

@lukel97 lukel97 left a comment

Choose a reason for hiding this comment

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

So far looks good. Will check this out locally soon and give it a spin!

@fendor fendor force-pushed the type-definition-request branch from ab5e0d9 to d18c006 Compare April 1, 2019 18:35
fendor
fendor commented Apr 1, 2019
View reviewed changes
@fendor fendor requested review from lorenzo and lukel97 and removed request for lorenzo April 1, 2019 18:42
@fendor fendor changed the title WIP: Add ReqTypeDefinition to reactor Add ReqTypeDefinition to reactor Apr 1, 2019
@fendor fendor requested a review from lorenzo April 1, 2019 18:55
mpickering
mpickering suggested changes Apr 3, 2019
View reviewed changes
Copy link
Collaborator

@mpickering mpickering left a comment

Choose a reason for hiding this comment

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

I think you should use the typeMap in order to find the type of the variable rather than using syb.

@fendor fendor force-pushed the type-definition-request branch 2 times, most recently from 6d9dff9 to 2d13993 Compare April 3, 2019 12:34
lukel97
lukel97 suggested changes Apr 3, 2019
View reviewed changes
@mpickering
Copy link
Collaborator

Looks ok to me now after @bubba's changes are taken into account.

@fendor fendor force-pushed the type-definition-request branch from b5acfa0 to 2d13993 Compare April 4, 2019 11:36
@alanz alanz modified the milestones: 2019-03, 2019-04 Apr 6, 2019
@fendor fendor mentioned this pull request Apr 6, 2019
Merged
@lorenzo
Copy link
Collaborator

lorenzo commented Apr 6, 2019

Do you need help adding the functional tests?

@fendor
Copy link
Collaborator Author

fendor commented Apr 6, 2019

Do you need help adding the functional tests?

I just thought, that I would have to add them to lsp-test itself.
Will do them soon.

@lukel97
Copy link
Collaborator

lukel97 commented Apr 7, 2019

Do you need help adding the functional tests?

I just thought, that I would have to add them to lsp-test itself.
Will do them soon.

Woops I forgot to bump and release lsp-test with the new typedef PR. I will get round to doing that soon! In the meantime it’s ok to use the submodule

@fendor
Copy link
Collaborator Author

fendor commented Apr 7, 2019 •
edited
Loading

Did not manage to use the lsp-test version that is patched for this pr.
So, just added test and wait for the version bump.
@bubba I will wait on the version bump.

fendor added 13 commits April 7, 2019 12:35
@fendor
Add ReqTypeDefinition to reactor
d2566d8
@fendor
Fix log message
f52db27
@fendor
Implement TypeDefinitionRequest
54bd2b9 
Works for variables, not for explicit constructors.
However, for explicit constructors, findDef works.

Implement tests for `data`, `newtype` and `type`.

For type defs, the original type definition will be found.
May be improved.

If the data type definition is not in scope, empty result will be sent.

Also add new dependency, hopefully this can be removed again
@fendor
Find sum type values
7b80721
@fendor
Revert changes to the test spec
3c2b82d
@fendor
Add documentation and use MaybeT
3cbd9e2
@fendor
Prefer ExcepT over MaybeT
e57858c 
Also add documentation and extract functions
@fendor
Add extra-dep for stack-8.2.1.yaml
c0d8abd
@fendor
Add tests for parameterized data types
3b234f1
@fendor
Simplify findTypeDef by utilising typeMap
925c996 
Undo changes that are not related to this feature
@fendor
Remove module lookup, since it can never be a module
acf2382
@fendor
Custom lsp-test to access findTypeDefinition
e976328
@fendor
Add Functional tests
7b58d49
@fendor fendor force-pushed the type-definition-request branch from 47f26f3 to 7b58d49 Compare April 7, 2019 10:42
@fendor
Copy link
Collaborator Author

fendor commented Apr 7, 2019

If the tests are green, this is ready to merge.

@fendor fendor force-pushed the type-definition-request branch from 07f0e14 to 610178b Compare April 7, 2019 20:34
@fendor fendor force-pushed the type-definition-request branch from 610178b to 625c61c Compare April 7, 2019 20:36
lukel97
lukel97 reviewed Apr 8, 2019
View reviewed changes
.gitmodules Outdated
@@ -37,3 +37,7 @@
url = https://github.com/ennocramer/floskell
# url = https://github.com/alanz/floskell

[submodule "submodules/lsp-test"]
path = submodules/lsp-test
url = https://github.com/bubba/lsp-test
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be removed I think

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, that slipped through, thank you!

@lukel97 lukel97 merged commit 3e38e39 into haskell:master Apr 8, 2019
@lukel97
Copy link
Collaborator

lukel97 commented Apr 8, 2019

Thanks! This is great. Another step towards full support for the protocol

@lorenzo
Copy link
Collaborator

lorenzo commented Apr 8, 2019

👏

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

@lukel97 lukel97 lukel97 approved these changes

@mpickering mpickering mpickering requested changes

@alanz alanz Awaiting requested review from alanz

@wz1000 wz1000 Awaiting requested review from wz1000

@lorenzo lorenzo Awaiting requested review from lorenzo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2019-04
Development

Successfully merging this pull request may close these issues.
5 participants
@fendor @mpickering @lorenzo @lukel97 @alanz