feat: supporting localDateTime function for datetime function #628

ilgolf · 2024-02-06T09:13:22Z

Motivation

JDSL doesn't support LOCAL DATETIME in JPQL

Modifications

I create localDateTime() & serializer to support LOCAL DATETIME function

Commit Convention Rule

Please commit your modification based by conventional commit:

Commit type	Description
feat	New Feature
fix	Fix bug
docs	Documentation only changed
ci	Change CI configuration
refactor	Not a bug fix or add feature, just refactoring code
test	Add Test case or fix wrong test case
style	Only change the code style(ex. white-space, formatting)
chore	It refers to minor tasks such as library version upgrade, typo correction, etc.

If you want to add some more commit type please describe it on the Pull Request

Result

Support LOCAL DATETIME function in JPQL

Closes

Support Datetime function LOCAL DATETIME #604

CLAassistant · 2024-02-06T09:13:27Z

All committers have signed the CLA.

codecov-commenter · 2024-02-06T09:33:11Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (4219a32) 91.97% compared to head (156bbc9) 91.96%.

Files	Patch %	Lines
...pql/serializer/impl/JpqlLocalDateTimeSerializer.kt	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #628      +/-   ##
==========================================
- Coverage   91.97%   91.96%   -0.02%     
==========================================
  Files         297      299       +2     
  Lines        3215     3222       +7     
  Branches      195      195              
==========================================
+ Hits         2957     2963       +6     
- Misses        204      205       +1     
  Partials       54       54

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

shouwn · 2024-02-07T00:10:19Z

...in/kotlin/com/linecorp/kotlinjdsl/querymodel/jpql/expression/impl/LocalDateTimeExpression.kt

+import com.linecorp.kotlinjdsl.querymodel.jpql.expression.Expression
+import java.time.LocalDateTime
+
+object LocalDateTimeExpression : Expression<LocalDateTime>


Please add the Internal annotation because this class belongs to an implementation.

@Internal object LocalDateTimeExpression : Expression<LocalDateTime>

shouwn · 2024-02-07T00:13:25Z

dsl/jpql/src/main/kotlin/com/linecorp/kotlinjdsl/dsl/jpql/Jpql.kt

+    /**
+     * Creates a datetime expression with the localDateTime.
+     */


I think the comment below are better. I've adopted the comments from the contributor of currentTime().

/** * Creates an expression that represents the local datetime. * * This is the same as ```LOCAL DATETIME```. */

shouwn · 2024-02-07T00:16:22Z

dsl/jpql/src/test/kotlin/com/linecorp/kotlinjdsl/dsl/jpql/LocalDateTimeExpressionDslTest.kt

+class LocalDateTimeExpressionDslTest {
+
+    @Test
+    fun `localDateTime to support LOCAL DATETIME in jpql`() {


To keep test names simple, we only write the name of the target method in the test name unless there are multiple test cases.

cj848 · 2024-02-07T00:55:49Z

docs/en/jpql-with-kotlin-jdsl/expressions.md

@@ -228,7 +228,7 @@ Kotlin JDSL provides a series of functions to support built-in functions in JPA.
 | CURRENT\_TIMESTAMP | not yet      |
 | LOCAL DATE         | not yet      |
 | LOCAL TIME         | not yet      |
-| LOCAL DATETIME     | not yet      |
+| LOCAL DATETIME     | support      |


functions that are already supported should be deleted.

cj848 · 2024-02-07T00:55:54Z

docs/ko/jpql-with-kotlin-jdsl/expressions.md

@@ -226,7 +226,7 @@ Kotlin JDSL은 JPA에서 제공하는 여러 함수들을 지원하기 위함
 | CURRENT\_TIMESTAMP | not yet      |
 | LOCAL DATE         | not yet      |
 | LOCAL TIME         | not yet      |
-| LOCAL DATETIME     | not yet      |
+| LOCAL DATETIME     | support      |


functions that are already supported should be deleted.

shouwn · 2024-02-08T00:00:59Z

...in/kotlin/com/linecorp/kotlinjdsl/render/jpql/serializer/impl/JpqlLocalDateTimeSerializer.kt

+        writer.writeParentheses {
+            delegate.serialize(part, writer, context)
+        }


I checked that LOCAL DATETIME does not need parentheses. Please remove the parentheses.

shouwn · 2024-02-08T00:02:14Z

...l/jpql/src/test/kotlin/com/linecorp/kotlinjdsl/querymodel/jpql/expression/ExpressionsTest.kt

@@ -887,4 +888,15 @@ class ExpressionsTest : WithAssertions {

        assertThat(actual).isEqualTo(expected)
    }
+
+    @Test
+    fun `localDateTime to support LOCAL DATETIME in jpql`() {


As with the DSL test, please simplify the test name.

shouwn · 2024-02-08T00:03:24Z

...model/jpql/src/main/kotlin/com/linecorp/kotlinjdsl/querymodel/jpql/expression/Expressions.kt

+    /**
+     * Creates an expression that represents the local datetime in jpql.
+     */


Please align the comment with DSL.

shouwn · 2024-02-08T00:04:35Z

Please sign our Contributor License Agreement.

shouwn · 2024-02-08T07:14:39Z

Thanks for your help! I'll fix some formatting after the merge!

This reverts commit 6d838d6, reversing changes made to 4219a32.

shouwn · 2024-02-08T07:33:03Z

@ilgolf Sorry, I didn't see that the base branch was main and merged it. I reverted the merge on the main branch, could you please create PR to the develop branch?

ilgolf · 2024-02-08T09:36:59Z

okay, sorry i miss... @shouwn

feat: supporting localDateTime function for datetime function

1bc08e1

ilgolf requested review from pickmoment, huisam, cj848 and shouwn as code owners February 6, 2024 09:13

shouwn reviewed Feb 7, 2024

View reviewed changes

cj848 reviewed Feb 7, 2024

View reviewed changes

fix: modify docs & comment and method name

fe672fe

ilgolf requested review from cj848 and shouwn February 7, 2024 11:30

fix: modify comment & method name

9167ca0

shouwn reviewed Feb 8, 2024

View reviewed changes

fix: modify comment & method name

156bbc9

shouwn approved these changes Feb 8, 2024

View reviewed changes

shouwn merged commit 6d838d6 into line:main Feb 8, 2024
4 checks passed

shouwn added a commit that referenced this pull request Feb 8, 2024

Revert "Merge pull request #628 from ilgolf/feat/LocalDateTime"

755c8d5

This reverts commit 6d838d6, reversing changes made to 4219a32.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: supporting localDateTime function for datetime function #628

feat: supporting localDateTime function for datetime function #628

ilgolf commented Feb 6, 2024

CLAassistant commented Feb 6, 2024 •

edited

Loading

codecov-commenter commented Feb 6, 2024 •

edited

Loading

shouwn Feb 7, 2024

shouwn Feb 7, 2024

shouwn Feb 7, 2024

cj848 Feb 7, 2024

cj848 Feb 7, 2024

shouwn Feb 8, 2024

shouwn Feb 8, 2024

shouwn Feb 8, 2024

shouwn commented Feb 8, 2024

shouwn commented Feb 8, 2024

shouwn commented Feb 8, 2024

ilgolf commented Feb 8, 2024

feat: supporting localDateTime function for datetime function #628

feat: supporting localDateTime function for datetime function #628

Conversation

ilgolf commented Feb 6, 2024

Motivation

Modifications

Commit Convention Rule

Result

Closes

CLAassistant commented Feb 6, 2024 • edited Loading

codecov-commenter commented Feb 6, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shouwn commented Feb 8, 2024

shouwn commented Feb 8, 2024

shouwn commented Feb 8, 2024

ilgolf commented Feb 8, 2024

CLAassistant commented Feb 6, 2024 •

edited

Loading

codecov-commenter commented Feb 6, 2024 •

edited

Loading