Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hover/go-to-def/refs for options #1783

Merged
merged 1 commit into from
Nov 7, 2022
Merged

Conversation

digama0
Copy link
Collaborator

@digama0 digama0 commented Oct 27, 2022

Now, clicking on foo in set_option foo true takes you to the register_option declaration for foo, if one exists; hover shows the docstring for the declaration, or the option's description; and find references on the declaration takes you to uses of the set_option.

@@ -175,6 +175,9 @@ example : Nat → Nat → Nat :=
x
--^ textDocument/hover

-- textDocument/definition -- removed because the result is platform-dependent
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately this is another of those tests that can't be written: This test jumps into the location of the lean 4 sources, which is different for the different CI runners and also for me locally. The goTo.lean tests fix this by only doing intra-file jumps, but options must be declared in a separate file so we have the usual issue about tests being single-file only.

Copy link
Member

Choose a reason for hiding this comment

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

Can we just replace the lean4 source directory by the empty string like we do for the tests?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know if this is desirable. We try to avoid source location references into src/ from tests since they are far from stable. It's really annoying when supposedly unrelated tests suddenly fail.

@gebner gebner added the awaiting-author Waiting for PR author to address issues label Nov 1, 2022
@digama0
Copy link
Collaborator Author

digama0 commented Nov 3, 2022

I don't know what is being asked of me here. I think the test scripts need to be rewritten in some way?

@gebner
Copy link
Member

gebner commented Nov 7, 2022

I would have liked to have the test included. To be clear my suggestion was to add a sed "s/$LEAN_DIR/\$LEAN_DIR/g" to tests/common.sh. It's indeed annoying if a test fails because of an added line in unrelated code, but it's even more annoying if LSP features regress without anybody noticing.

But I don't want to block the PR on that.

@Kha do you have any issues with this PR?

@gebner gebner added server Affects the Lean server code and removed awaiting-author Waiting for PR author to address issues labels Nov 7, 2022
@Kha Kha added the changelog label Nov 7, 2022
@Kha Kha changed the title feat: hover/go-to-def/refs for options Add hover/go-to-def/refs for options Nov 7, 2022
@Kha Kha merged commit 4fefb20 into leanprover:master Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
server Affects the Lean server code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants