Skip to content

@ConfigProperty code action when property is not set#167

Merged
rgrunber merged 1 commit intoeclipse-lsp4mp:masterfrom
angelozerr:configproperty-notset-codeaction
Sep 10, 2021
Merged

@ConfigProperty code action when property is not set#167
rgrunber merged 1 commit intoeclipse-lsp4mp:masterfrom
angelozerr:configproperty-notset-codeaction

Conversation

@angelozerr
Copy link
Copy Markdown
Contributor

@angelozerr angelozerr commented Aug 23, 2021

@ConfigProperty code action when property is not set

Fixes #147

Signed-off-by: azerr azerr@redhat.com

@angelozerr angelozerr force-pushed the configproperty-notset-codeaction branch 5 times, most recently from 7572fac to 953f698 Compare August 27, 2021 08:15
@angelozerr
Copy link
Copy Markdown
Contributor Author

angelozerr commented Aug 27, 2021 •
edited
Loading

Ok now I support insert defaultValue attribute in Java file and insert property inside available properties files.

Here a demo:

CodeActionInsertPropertyDemo

Please note that when you use properties code action, you need to save the file at hand (I don't know how to do that with code action).

You can start to play with my PR and review it, but I need to:

  • write tests
  • manage correctly ConfigProperty with ConfigProperties.

@angelozerr angelozerr force-pushed the configproperty-notset-codeaction branch 2 times, most recently from c71cbb4 to 7363caf Compare August 27, 2021 13:35
@angelozerr
Copy link
Copy Markdown
Contributor Author

angelozerr commented Aug 27, 2021

In this PR I migrat eto LSP4J 0.11.0 because I'm using Diagnostic#data. Please note that this PR requires too redhat-developer/vscode-microprofile#68 which migrate to vascode-languageclient 0.7.0 to use Diagnostic#data too.

@angelozerr angelozerr force-pushed the configproperty-notset-codeaction branch from 7363caf to cbe4f69 Compare August 27, 2021 13:50
@angelozerr
Copy link
Copy Markdown
Contributor Author

angelozerr commented Aug 27, 2021

CodeAction with ConfigProperties should work now:

image

@angelozerr angelozerr force-pushed the configproperty-notset-codeaction branch 6 times, most recently from 816b266 to 9bba076 Compare August 30, 2021 09:15
@angelozerr
Copy link
Copy Markdown
Contributor Author

angelozerr commented Aug 30, 2021

I wrote tests, this PR can be reviewed now.

@angelozerr angelozerr marked this pull request as ready for review August 30, 2021 09:16
@angelozerr angelozerr force-pushed the configproperty-notset-codeaction branch 5 times, most recently from 4bbcbd5 to 4ae38ed Compare August 30, 2021 10:13
@angelozerr angelozerr requested a review from rgrunber August 30, 2021 11:24
@rgrunber
Copy link
Copy Markdown
Contributor

rgrunber commented Sep 8, 2021

Overall things look good. I just want to go over InsertAnnotationAttributeProposal.java a little more closely. I think we can live with the fact that one must save the properties file or else the same code action will continue to be suggested.

@angelozerr angelozerr force-pushed the configproperty-notset-codeaction branch 2 times, most recently from 97b0ea4 to 5847a94 Compare September 9, 2021 07:27
@angelozerr
Copy link
Copy Markdown
Contributor Author

angelozerr commented Sep 9, 2021

I just want to go over InsertAnnotationAttributeProposal.java a little more closely.

Ok I'm waiting for your feedback.

think we can live with the fact that one must save the properties file or else the same code action will continue to be suggested.

Yes it can be annoying but I don't know how to save with a standard command the file.

rgrunber
rgrunber approved these changes Sep 9, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Overall, this works for me. Only one small issue maybe worth addressing, and I just commented on why linked proposals likely aren't doing anything, but would be good to keep for future compatibility.

Feel free to merge when ready.

Comment thread ...org/eclipse/lsp4mp/jdt/core/java/corrections/proposal/InsertAnnotationAttributeProposal.java Outdated
Comment thread ...main/java/org/eclipse/lsp4mp/jdt/internal/config/java/NoValueAssignedToPropertyQuickFix.java
if (lineSeparator == null) {
lineSeparator = System.lineSeparator();
}
String propertyName = getPropertyName(diagnostic, context);
Copy link
Copy Markdown
Contributor

@rgrunber rgrunber Sep 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I noticed the code action shows up even when name is empty. (eg. @ConfigProperty(name=""). Is it worth check for empty here to avoid this ? Generating just an = in the file seemed a bit odd.

Note that we can't just silence the diagnostic because inserting defaultValue is still valid.

Copy link
Copy Markdown
Contributor Author

@angelozerr angelozerr Sep 10, 2021

Choose a reason for hiding this comment

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

@rgrunber after playing with Quarkus application,it seems that @ConfigProperty(name="") is not allowed:

image

It means that we should report an another diagnostic message for this usecase with a code=null instead of NO_VALUE_ASSIGNED_TO_PROPERTY and the problem will be fixed. I created the issue for that #176 which will fix your comment below.

Copy link
Copy Markdown
Contributor

@rgrunber rgrunber Sep 10, 2021

Choose a reason for hiding this comment

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

Bah, I was hoping it wouldn't be required, and we could get away with just silencing that code action, but fair enough.

@angelozerr angelozerr force-pushed the configproperty-notset-codeaction branch from 5847a94 to 5d86369 Compare September 9, 2021 16:58
@angelozerr
@ConfigProperty code action when property is not set
5d86369 
Fixes eclipse-lsp4mp#147

Signed-off-by: azerr <azerr@redhat.com>
@angelozerr angelozerr mentioned this pull request Sep 10, 2021
Closed
@rgrunber rgrunber merged commit 9d00aaf into eclipse-lsp4mp:master Sep 10, 2021
@rgrunber rgrunber mentioned this pull request Sep 10, 2021
Closed
@angelozerr angelozerr mentioned this pull request Sep 20, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rgrunber rgrunber rgrunber approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

@ConfigProperty code action when property is not set

2 participants

@angelozerr @rgrunber
⚡