Skip to content

Fixed bug with missing definition hover for multiple annotation members#216

Merged
angelozerr merged 1 commit intoeclipse-lsp4mp:masterfrom
AlexXuChen:pr214-fix
Dec 7, 2021
Merged

Fixed bug with missing definition hover for multiple annotation members#216
angelozerr merged 1 commit intoeclipse-lsp4mp:masterfrom
AlexXuChen:pr214-fix

Conversation

@AlexXuChen
Copy link
Copy Markdown
Contributor

Fixed bug with missing definition hover for multiple annotation members

References #214 (comment)

Signed-off-by: Alexander Chen alchen@redhat.com

@angelozerr
Copy link
Copy Markdown
Contributor

Could you write a test please.

@AlexXuChen
Copy link
Copy Markdown
Contributor Author

Could you write a test please.

There are existing tests for this feature (e.g. hover), the definition related tests are part of quarkus-ls: redhat-developer/quarkus-ls/pull/458

@angelozerr
Copy link
Copy Markdown
Contributor

As this PR belongs to LSP4MP we should have tests (even if we have some tests on Quarkus LS side). In other words we should have tests:

  • for hover with ConfigProperty name with one member
    • (ex : ConfigProperty(name = "greeting.mess|age"))
    • and two members (check existing tests). For two members I will write a test with ConfigProperty(defaultValue = "hello", name = "greeting.mess|age")
  • for definition with ConfigProperty like hover.

@AlexXuChen
Copy link
Copy Markdown
Contributor Author

AlexXuChen commented Dec 6, 2021

As this PR belongs to LSP4MP we should have tests (even if we have some tests on Quarkus LS side). In other words we should have tests:

As I mentioned, there are existing tests for hover: https://github.com/eclipse/lsp4mp/blob/master/microprofile.jdt/org.eclipse.lsp4mp.jdt.test/src/main/java/org/eclipse/lsp4mp/jdt/core/config/java/MicroProfileConfigJavaHoverTest.java, which already handle the multiple member cases, and definition: https://github.com/eclipse/lsp4mp/blob/master/microprofile.jdt/org.eclipse.lsp4mp.jdt.test/src/main/java/org/eclipse/lsp4mp/jdt/core/config/java/MicroProfileConfigJavaDefinitionTest.java. For this pr, since it is on the lsp4mp side, only name is supported with ConfigProperty definition hover, and this test case is already handled: https://github.com/eclipse/lsp4mp/blob/ec5f1ab23b1d3f08f2bd6b60f37bea15a3069d6d/microprofile.jdt/org.eclipse.lsp4mp.jdt.test/src/main/java/org/eclipse/lsp4mp/jdt/core/config/java/MicroProfileConfigJavaDefinitionTest.java#L90

@AlexXuChen AlexXuChen force-pushed the pr214-fix branch 3 times, most recently from 69e0f4c to 29e96d6 Compare December 6, 2021 19:09
@angelozerr
Copy link
Copy Markdown
Contributor

As I mentioned, there are existing tests

Oh my bad, you are totaly right, sorry I see too quickly your PR and comments.

@AlexXuChen AlexXuChen force-pushed the pr214-fix branch 2 times, most recently from d23f287 to e996ec5 Compare December 6, 2021 22:57
@AlexXuChen AlexXuChen requested a review from angelozerr December 7, 2021 14:08
Signed-off-by: Alexander Chen <alchen@redhat.com>
@angelozerr angelozerr merged commit 36c78ec into eclipse-lsp4mp:master Dec 7, 2021
@angelozerr
Copy link
Copy Markdown
Contributor

Good job @AlexXuChen !

@AlexXuChen AlexXuChen deleted the pr214-fix branch December 7, 2021 15:58
@rgrunber rgrunber added this to the 0.4.0 milestone Dec 7, 2021
@rgrunber rgrunber added the bug Something isn't working label Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants