redhat-developer / quarkus-ls

Language server for Quarkus tooling
Eclipse Public License 2.0
43 stars 15 forks source link

Property reference syntax should also support the dollar sign #425

Closed rgrunber closed 2 years ago

rgrunber commented 2 years ago

In the @Scheduled annotation, cron = {cron.expr} will refer to a referenced property cron.expr. Hovering over it will confirm this. However, ${cron.expr} is also a valid syntax, but hovering over it does not produce anything.

The issue is due to https://github.com/eclipse/lsp4mp/blob/master/microprofile.jdt/org.eclipse.lsp4mp.jdt.core/src/main/java/org/eclipse/lsp4mp/jdt/core/java/hover/PropertiesHoverParticipant.java#L161 not handling $.

This also has an odd side effect. Take any name member of @ConfigProperty, and reference a valid property as follows : @ConfigProperty(name="{greeting.message}. An warning will show to indicate that the value doesn't exist, yet the hover will reference greeting.message in the configuration file!

Are we ok with this ? It looks like to avoid this we would have to move the logic that accepts the curly brace/dollar somewhere else, and potentially enable/disable it based on the annotation/member On the other hand, this might be a nice side effect also.

angelozerr commented 2 years ago

I understand the problem and I think PropertiesHoverParticipant should be customizable by adding a new parameter in the constructor Function<String, String> propertyReplacer and use it in https://github.com/eclipse/lsp4mp/blob/master/microprofile.jdt/org.eclipse.lsp4mp.jdt.core/src/main/java/org/eclipse/lsp4mp/jdt/core/java/hover/PropertiesHoverParticipant.java#L161

We should have a default replacer which return the property key.

rgrunber commented 2 years ago

Nice. I think I like this approach. I was thinking just add a method to the base class that ca be overridden, to strip out "reference" characters if applicable, but this also works.

We would also need to do the same thing in https://github.com/eclipse/lsp4mp/blob/master/microprofile.jdt/org.eclipse.lsp4mp.jdt.core/src/main/java/org/eclipse/lsp4mp/jdt/core/java/definition/PropertiesDefinitionParticipant.java#L56 to support going to definition of a {..} or ${..} property value. This would have implications for https://github.com/redhat-developer/quarkus-ls/issues/377

rgrunber commented 2 years ago

Closing as duplicate of #401