-
Notifications
You must be signed in to change notification settings - Fork 408
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 final modifier where possible #1234
Conversation
test this please |
@NikolasKomonen 'test this please' is a request for Jenkins - https://ci.eclipse.org/ls/job/jdt-ls-pr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NikolasKomonen could you check the following test - org.eclipse.jdt.ls.core.internal.handlers.DocumentLifeCycleHandlerTest.testRemoveDeadCodeAfterIf()
See https://ci.eclipse.org/ls/job/jdt-ls-pr/1681/testReport/org.eclipse.jdt.ls.core.internal.handlers/DocumentLifeCycleHandlerTest/testRemoveDeadCodeAfterIf/
46ad617
to
58b00bf
Compare
The behavior is really surprising, since, it makes all variables in the file final. So the label is definitely confusing here. |
@fbricon I've submitted the issue to fix this: https://bugs.eclipse.org/bugs/show_bug.cgi?id=552490 |
...e.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/text/correction/QuickAssistProcessor.java
Show resolved
Hide resolved
...s.tests/src/org/eclipse/jdt/ls/core/internal/correction/ModifierCorrectionsQuickFixTest.java
Outdated
Show resolved
Hide resolved
58b00bf
to
d069363
Compare
@fbricon resolved the changes |
test this please |
Fixes redhat-developer/vscode-java#774 One thing that can be improved is the message "Add final", but that relies on a change from jdt code. Signed-off-by: Nikolas Komonen <[email protected]>
d069363
to
eea751c
Compare
test this please |
@Eskibear can you please check this change is compatible with the current refactoring of code actions? |
In this PR, it creates a My next step is to move almost all proposals (except |
there's no diagnostic, this applies to the whole class. So we can merge it? |
No problem, it’s not conflicting with my work. I suggest to change the kind to ‘quickassist’ or ‘source’(it sounds more like this kind, but in vscode it will only appear when you click SourceAction...) . Anyway it’s not conflicting, you can simply merge it and I can help change it later. |
ok thanks, @NikolasKomonen is only working part time, so I'll merge it now and let you do the necessary changes later |
Fixes redhat-developer/vscode-java#774
One thing that can be improved is the message "Add final", but that relies on
a change from jdt code.
Signed-off-by: Nikolas Komonen [email protected]