Skip to content

Conversation

MoadElfatihi
Copy link
Contributor

@MoadElfatihi MoadElfatihi commented Sep 6, 2025

Hello,

This PR is about some changes to use jdk 17 syntaxe (no behavioral change).

Just a contribution to give a hand with ongoing codebase cleanup work 👍

Thanks

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.


@hibernate-github-bot
Copy link

hibernate-github-bot bot commented Sep 6, 2025

Thanks for your pull request!

This pull request does not follow the contribution rules. Could you have a look?

❌ All commit messages should start with a JIRA issue key matching pattern HHH-\d+
    ↳ Offending commits: [91b429f]

› This message was automatically generated.

Copy link
Member

@gavinking gavinking left a comment

Choose a reason for hiding this comment

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

OK, thanks.

Many of these changes I like, but:

  1. I'm not at all a fan of getting rid of else everywhere. To me this just disrupts the symmetry of the code. I mean false and true are symmetric, why should true be more nested? I do, on the other hand, like the ?: operator, which feels more "functional" to me.
  2. I have not generally been replacing everything with var. I'm usually just doing it for longer type names and especially generic types. I definitely don't want to go through the whole code base and replace primitives, String, Integer, and Object with var everywhere. That just doesn't buy us anything, really. So for consistency, I would prefer that we avoid var in those "trivial" cases.
  3. I have not been writing instanceof final because it has too much tendency to make long if conditions even longer. For consistency across the code base, let's stick with just instanceof.

@MoadElfatihi MoadElfatihi marked this pull request as draft September 14, 2025 16:50
@MoadElfatihi MoadElfatihi marked this pull request as ready for review September 14, 2025 18:52
@MoadElfatihi MoadElfatihi marked this pull request as draft September 14, 2025 19:04
@MoadElfatihi MoadElfatihi marked this pull request as ready for review September 14, 2025 19:27
@MoadElfatihi
Copy link
Contributor Author

OK, thanks.

Many of these changes I like, but:

  1. I'm not at all a fan of getting rid of else everywhere. To me this just disrupts the symmetry of the code. I mean false and true are symmetric, why should true be more nested? I do, on the other hand, like the ?: operator, which feels more "functional" to me.
  2. I have not generally been replacing everything with var. I'm usually just doing it for longer type names and especially generic types. I definitely don't want to go through the whole code base and replace primitives, String, Integer, and Object with var everywhere. That just doesn't buy us anything, really. So for consistency, I would prefer that we avoid var in those "trivial" cases.
  3. I have not been writing instanceof final because it has too much tendency to make long if conditions even longer. For consistency across the code base, let's stick with just instanceof.

OK, thanks.

Many of these changes I like, but:

  1. I'm not at all a fan of getting rid of else everywhere. To me this just disrupts the symmetry of the code. I mean false and true are symmetric, why should true be more nested? I do, on the other hand, like the ?: operator, which feels more "functional" to me.
  2. I have not generally been replacing everything with var. I'm usually just doing it for longer type names and especially generic types. I definitely don't want to go through the whole code base and replace primitives, String, Integer, and Object with var everywhere. That just doesn't buy us anything, really. So for consistency, I would prefer that we avoid var in those "trivial" cases.
  3. I have not been writing instanceof final because it has too much tendency to make long if conditions even longer. For consistency across the code base, let's stick with just instanceof.

Dear @gavinking ,

Thank you for the attention you gave to this PR and sorry for the extra mental load it might have caused.
I really appreciate your detailed feedback and I update the PR accordingly:

  1. Restore the else blocks for clarity and symmetry.
  2. Limit the use of var to cases where it improves readability ( I had noticed one of your commits titled "use more var", and I misunderstood it as a more general guideline. Thanks for clarifying the intended scope)
  3. Avoid instanceof final and stick to plain instanceof for consistency.

Thanks @gavinking 🙏

Copy link
Member

@gavinking gavinking left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@gavinking gavinking merged commit b1411aa into hibernate:main Sep 22, 2025
22 of 24 checks passed
@gavinking
Copy link
Member

Thank you @MoadElfatihi

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.

2 participants