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.

Sign up for GitHub

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

Jump to bottom

Code Cleanup: Java 16 instanceof pattern matching #705

Closed
wants to merge 1 commit into from
Closed

Code Cleanup: Java 16 instanceof pattern matching #705

wants to merge 1 commit into from

Conversation

alshamams
Copy link
Contributor

Pattern matching using instanceof operator in Java generally involves testing whether an object belongs to a certain type, and further converting the object to that type depending on the test result.

From Java 16 onwards, this conversion step is avoided by using the type pattern as the second operand of instance operator itself. This helps make the code more concise.

This commit has tried to implement the above mentioned code change throughout.

Ref: #592

gireeshpunathil
gireeshpunathil reviewed Aug 10, 2023
View reviewed changes
apitools/org.eclipse.pde.api.tools.annotations/.settings/org.eclipse.jdt.ui.prefs
@@ -1,55 +1,145 @@
cleanup.add_all=false
Copy link
Contributor

Choose a reason for hiding this comment

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

did you mean these changes (in preferences) to be made permanent?

akurtakov
akurtakov reviewed Aug 10, 2023
View reviewed changes
...ua.ui/src/org/eclipse/pde/internal/ua/ui/editor/cheatsheet/comp/CompCSMasterTreeSection.java
// Cheat sheet view will log an error loading simple cheat
// sheets
IStorageEditorInput storageInput = (IStorageEditorInput) input;
} else if (input instanceof IStorageEditorInput storageInput) {
Copy link
Member

Choose a reason for hiding this comment

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

Please keep the comment .

Copy link
Member

Choose a reason for hiding this comment

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

Is this a bug in the cleanup command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this a bug in the cleanup command?

Hi @vik-chand ,
I did not remove the comment manually. It was done as part of the code cleanup.

@iloveeclipse
Copy link
Member

This commit has tried to implement the above mentioned code change throughout.

Was it done manually or via some refactoring command?
Seeing 400 changed files makes me nervous, especially so close to M3.

@github-actions
Copy link

Test Results

   231 files   -      15     231 suites   - 15   36m 30s ⏱️ - 17m 31s
   635 tests  - 2 663     635 ✔️  - 2 637  0 💤  - 24  0 ❌ ±0 
2 076 runs   - 8 115  2 076 ✔️  - 8 041  0 💤  - 72  0 ❌ ±0 

Results for commit fbbe725. ± Comparison against base commit d7228a1.

This pull request removes 2663 tests. 
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingAnalysisAntTaskTests ‑ test1
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingAnalysisAntTaskTests ‑ test2
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingAnalysisAntTaskTests ‑ test3
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingAnalysisAntTaskTests ‑ test4
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingAnalysisAntTaskTests ‑ test5
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingAnalysisAntTaskTests ‑ test6
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingAnalysisAntTaskTests ‑ test7
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingApiFreezeAntTaskTests ‑ test1
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingApiFreezeAntTaskTests ‑ test2
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingApiFreezeAntTaskTests ‑ test3
…

@alshamams
Copy link
Contributor Author

This commit has tried to implement the above mentioned code change throughout.

Was it done manually or via some refactoring command? Seeing 400 changed files makes me nervous, especially so close to M3.

Hi @iloveeclipse ,
This was done by using code cleanup from preferences, and this feature for pattern matching is readily available.

akurtakov
akurtakov reviewed Aug 10, 2023
View reviewed changes
ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/PDEPlugin.java
if (e instanceof CoreException) {
// Re-use status only if it has attached exception with the stack trace
CoreException ce = (CoreException) e;
if (e instanceof CoreException ce) {
Copy link
Member

Choose a reason for hiding this comment

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

Please keep the comment.

@akurtakov
Copy link
Member

I can only second that such patches should be done in smaller chunks e.g. one PR per project to make reviewing actually possible.

@alshamams
Copy link
Contributor Author

@iloveeclipse @akurtakov Henceforth I shall try to focus on making smaller commits. Should I revert and convert this into individual commits, for each project ?

@akurtakov
Copy link
Member

Please do. That would allow us to deliver incrementally.

@alshamams
Copy link
Contributor Author

Hi @iloveeclipse @akurtakov @vik-chand @gireeshpunathil
I will be closing this PR, since I will be creating a new PR for each project, and dividing the entire work as you have suggested.

@alshamams alshamams closed this Aug 12, 2023
@vik-chand
Copy link
Member

Sounds good ! Ideally we would want such incremental changes in M1/M2. M3 and RC1 are too close for comfort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vik-chand vik-chand vik-chand left review comments

@gireeshpunathil gireeshpunathil gireeshpunathil left review comments

@akurtakov akurtakov akurtakov left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alshamams @iloveeclipse @akurtakov @vik-chand @gireeshpunathil