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

Small checks and behavior differences for refactorings and assists #921

Merged
merged 2 commits into from
Dec 1, 2023

Conversation

robstryker
Copy link
Contributor

Part 1

There will be 2 test failures but i forget what they are so I'll let Jenkins discover it and fix them.

What it does

How to test

Author checklist

@robstryker robstryker marked this pull request as draft November 13, 2023 15:16
@robstryker robstryker marked this pull request as ready for review November 13, 2023 17:40
@@ -161,7 +161,7 @@ public RefactoringStatus checkActivationBasics(CompilationUnit rootNode) throws
RefactoringStatus result= new RefactoringStatus();
fRootNode= rootNode;

fAnalyzer= new SurroundWithTryCatchAnalyzer(fCUnit, fSelection);
fAnalyzer= new SurroundWithTryCatchAnalyzer(fCUnit, fSelection, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to be a preference which jdt-ls can set to true by default and false otherwise. I think there are scenarios where a user would want to handle the exception for the method (e.g. to log something that isn't ever supposed to happen and a similar-type exception is actually expected in subsequent code). Removing the case forces the user have to add it manually vs deleting it if it is extraneous.

Copy link
Contributor

Choose a reason for hiding this comment

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

Rob addressed this by introducing a new field in the class.

@robstryker
Copy link
Contributor Author

Reverted the test changes and behavior. Since this is delayed, I will keep making any changes necessary for jdt.ls. Marking as draft.

…r headless consumers (jdt.ls)

Signed-off-by: Rob Stryker <[email protected]>
Copy link
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.

Change looks fine to me. This is mainly adding some protected classes allowing for some possible customization in refactoring behaviour.

My only concern was about the code where you retrieve only the first element of a composite change. However, looking into the (2) implementations of RefactoringCorrectionProposalCore, none appear to be returning a composite change. Furthermore looking at most classes that implement Refactoring, they seem to be returning a CompilationUnitChange, which is a TextChange.

Signed-off-by: Rob Stryker <[email protected]>
@rgrunber rgrunber added this to the 4.31 M1 milestone Dec 1, 2023
@rgrunber rgrunber dismissed jjohnstn’s stale review December 1, 2023 22:04

Change has been addressed.

@rgrunber rgrunber merged commit 4e1aea2 into eclipse-jdt:master Dec 1, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants