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 final modifier where possible #1234

Merged
merged 1 commit into from
Nov 12, 2019

Conversation

NikolasKomonen
Copy link
Contributor

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]

@fbricon fbricon requested a review from snjeza October 25, 2019 15:02
@snjeza
Copy link
Contributor

snjeza commented Oct 25, 2019

test this please

@snjeza
Copy link
Contributor

snjeza commented Oct 25, 2019

test this please
with unit tests or by hand? Both are working for me.

@NikolasKomonen 'test this please' is a request for Jenkins - https://ci.eclipse.org/ls/job/jdt-ls-pr

Copy link
Contributor

@snjeza snjeza left a 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/

@fbricon
Copy link
Contributor

fbricon commented Oct 28, 2019

The behavior is really surprising, since, it makes all variables in the file final. So the label is definitely confusing here.
Either we change the behavior to only apply to the selected variable, or we need to change the label to "Make all variables final"

@NikolasKomonen
Copy link
Contributor Author

@fbricon I've submitted the issue to fix this: https://bugs.eclipse.org/bugs/show_bug.cgi?id=552490
They just need to swap the message with a better one.

@NikolasKomonen
Copy link
Contributor Author

@fbricon resolved the changes

@snjeza
Copy link
Contributor

snjeza commented Nov 9, 2019

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]>
@snjeza
Copy link
Contributor

snjeza commented Nov 11, 2019

test this please

@fbricon
Copy link
Contributor

fbricon commented Nov 12, 2019

@Eskibear can you please check this change is compatible with the current refactoring of code actions?

@Eskibear
Copy link
Contributor

In this PR, it creates a FixCorrectionProposal which is by default of kind 'quickfix'. A code action is a 'quickfix' if there's a corresponding diagnotic information like an error/warning/info. If there is not, personally I think it's more like a 'quickassist', then it should stay here in QuickAssistProcessor.

My next step is to move almost all proposals (except getAssignParamToFieldProposals and getAssignAllParamsToFieldsProposals) from QuickAssistProcessor into RefactorProcessor, as they are actually of kind 'refactor'.

@fbricon
Copy link
Contributor

fbricon commented Nov 12, 2019

there's no diagnostic, this applies to the whole class. So we can merge it?

@Eskibear
Copy link
Contributor

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.

@fbricon
Copy link
Contributor

fbricon commented Nov 12, 2019

ok thanks, @NikolasKomonen is only working part time, so I'll merge it now and let you do the necessary changes later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to add 'final' modifier where possible.
4 participants