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

feat: support crud method metadata of spring #665

Merged
merged 2 commits into from
Mar 6, 2024

Conversation

shouwn
Copy link
Member

@shouwn shouwn commented Mar 4, 2024

Motivation

  • Kotlin JDSL does not support CrudMethodMetadata of Spring Data.

Modifications

  • Support @Lock of Spring Data.
  • Support @QueryHints of Spring Data.

Closes

}

private fun setLockMode(query: Query, lockMode: LockModeType?) {
if (lockMode != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (lockMode != null) {
lockMode?.run {

how about this style?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think if is better in this case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok. It's a style so it doesn't really matter
However, for null checking, how about !== null (reference comparison) rather than != null?

Copy link
Member Author

Choose a reason for hiding this comment

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

How do they differ? What's the difference between the two? I checked with Java compilation and there is no difference between lockMode?.run, if (lockMode != null) and if (lockMode !== null) for null checking.

Copy link
Collaborator

Choose a reason for hiding this comment

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

== is an equals comparison, === is a reference comparison.
However, as far as I know, this is according to the Kotlin language specification and may be the same when converted to Java.
In the case of variable?.run , the run method is an inline method, so we expect the code to be converted to Java code as a result, similar to the if code.

As a result, === conveys a clear intent about which values require reference comparison.

Copy link
Member Author

@shouwn shouwn Mar 6, 2024

Choose a reason for hiding this comment

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

You're not talking about functionality, you're talking about how to express the code. I think it's not a bad idea to use reference equality to indicate whether the reference of this object is null or not. But if you have to enforce it for the whole project, I don't think so.

Kotlin provides both structural equality and reference equality as ways of checking nullability. You can see this by looking at the code that Kotlin itself uses. They use structural equality in some places and reference equality in others to check the nullability.

==null case
===null case

So I don't want to force people to use === when checking the nullability.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the part that checks for nullability is relevant. There is no problem doing whatever you want. In addition to ?.run, I think it would be good to discuss the part about checking the enum with ===.
In the case of enums, I think === is more correct because it is a singleton and does not need to be checked with equals(==).

However, looking at kotlin documentation, it does not seem necessary.

I respect your opinion.

Copy link
Member Author

Choose a reason for hiding this comment

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

I appreciate your respect.

Regarding enums, I would rather use == than ===. Enums are singletons, but they are also objects. We use structural equality a lot as a way to compare two objects. That's why I think it's better to use == when comparing enums.

For example, suppose you have a method that looks like this:

fun compare(color1: Color, color2: Color): Boolean {
  TODO()
}

How would you compare them? When you use ===, it means that you know that the Color class is implemented as an Enum class.

What would happen if you changed the implementation of the Color class to a Sealed class instead of an Enum class? If you used ===, you would have to change the implementation.

This is because Enum is a way to implement an enumeration, and Kotlin provides Sealed classes as a way to implement an enumeration. That's why I prefer to use === instead of ==.

Copy link
Collaborator

@cj848 cj848 Mar 6, 2024

Choose a reason for hiding this comment

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

It's like a difference in perspective. If you change to a sealed class, there is a possibility that the logic will change a lot, and you will definitely have to recheck the automated tests you created, but I don't think there is a need to approach the issue of whether or not to fix the code. However, in the case of null checking inside Kotlin, there is documentation that === or == will be converted automatically, so there is no harm in following your convention unless there are other cases where you need to explicitly check the reference.

And since my review is approved, I think you can merge it when you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow... that's amazing. I'm learning.

@shouwn shouwn requested a review from cj848 March 4, 2024 23:17
@shouwn shouwn merged commit c59b4c3 into develop Mar 6, 2024
4 checks passed
@shouwn shouwn deleted the feature/spring-method-metadata branch March 6, 2024 23:12
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