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

Migrate to Java 17 switch #584

Merged
merged 1 commit into from
May 1, 2023

Conversation

preveen-stack
Copy link

@preveen-stack preveen-stack commented Apr 20, 2023

Replace legacy switch block with java 17 semantics
ref: https://openjdk.org/jeps/361

PS: This is my first PR in eclipse-pde.

@preveen-stack preveen-stack marked this pull request as ready for review April 20, 2023 12:30
@vogella
Copy link
Contributor

vogella commented Apr 20, 2023

Thanks.

Please squash the commits into one and avoid merge commits in future contriburtions.

@github-actions
Copy link

github-actions bot commented Apr 20, 2023

Test Results

     240 files  ±0       240 suites  ±0   54m 30s ⏱️ - 3m 19s
  3 278 tests ±0    3 254 ✔️ ±0  24 💤 ±0  0 ±0 
10 131 runs  ±0  10 059 ✔️ ±0  72 💤 ±0  0 ±0 

Results for commit 7ca2995. ± Comparison against base commit 59a2a32.

♻️ This comment has been updated with latest results.

@preveen-stack
Copy link
Author

preveen-stack commented Apr 20, 2023

@vogella Commits squashed

@preveen-stack
Copy link
Author

ECA completed

Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

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

Thanks for this.
From a coarse look it looks good, I only have one little more suggestion below.
Have not yet reviewed in detail.

@vogella
Copy link
Contributor

vogella commented Apr 27, 2023

LGTM

Any more suggestions @HannesWell ?

@preveen-stack preveen-stack changed the title Java 17 switch Migrate to Java 17 switch Apr 27, 2023
@HannesWell
Copy link
Member

LGTM

Any more suggestions @HannesWell ?

I'll review it tomorrow.

Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

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

The change looks good.
But at some places I think the use of switches is not so well suited and if/else/else-if statements are a better fit. But that was already the case before.

Nevertheless I changed those places to use if-statements instead.
In general I think many of the switch-expressions should be pattern-matches, but that will require Java-21 or even later (see #592).

Besides that I added missing default cases that caused warnings.

- changed to Java 17 Switch Expression
- replace switch-statements by if/else-statements where suitable

Co-authored-by: Hannes Wellmann <[email protected]>
@HannesWell HannesWell merged commit b1a4e1c into eclipse-pde:master May 1, 2023
@HannesWell
Copy link
Member

Thank you @preveen-stack.

@preveen-stack preveen-stack deleted the java_17_switch branch September 1, 2023 05:25
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.

4 participants