GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Switch to JSR 310 Date and Time API by dragneelfps · Pull Request #233 · jasync-sql/jasync-sql

Conversation

@dragneelfps

@dragneelfps dragneelfps commented May 28, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
  • Removes joda from encoding/decoding

  • common

  • mysql

  • postgres

continuation of #229

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ilya40umov and oshai reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@dragneelfps

dragneelfps commented May 28, 2021

Copy link
Copy Markdown
Contributor Author

Getting following in CI build.. :/

docker: Error response from daemon: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@oshai

oshai commented May 30, 2021

Copy link
Copy Markdown
Contributor

Hi, thanks for the PR!
There was some interest for it in the past. For reference see #131 and #157.
Ideally, if we could have both implementation as a non-breaking change it would be preferred.
But I understand this might complicate things a lot.
Since this is a breaking change we will need to bump the major version when merged.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dragneelfps

dragneelfps commented May 30, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

I could try to have Joda Time supported as well. But the thing is, do we really want to continue supporting it?
I prefer the major version upgrade. But seems odd going from 1.1.7 to 2.0.0.
Suggestions on how to move forward with this please?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@oshai

oshai commented Jun 1, 2021

Copy link
Copy Markdown
Contributor

Bump version to 2.0.0 sounds reasonable to me.
Please rebase changes from master again, build should work now in CI.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dragneelfps
dragneelfps force-pushed the master branch 2 times, most recently from a6c447a to 6ea3c8f Compare June 2, 2021 03:11
@oshai

oshai commented Jun 2, 2021

Copy link
Copy Markdown
Contributor

Note that some tests are still failing on CI.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dragneelfps

dragneelfps commented Jun 5, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

I fixed most. Just having issues understanding logic behind

jasync-sql/postgresql-async/src/test/java/com/github/aysnc/sql/db/integration/TimeAndDateSpec.kt

Line 151 in 152cdef

assertThat(dateTime.millis).isGreaterThan(915779106000L)

can you help, @oshai ?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@oshai

oshai commented Jun 5, 2021

Copy link
Copy Markdown
Contributor

I fixed most. Just having issues understanding logic behind

jasync-sql/postgresql-async/src/test/java/com/github/aysnc/sql/db/integration/TimeAndDateSpec.kt

Line 151 in 152cdef

assertThat(dateTime.millis).isGreaterThan(915779106000L)

can you help, @oshai ?

I didn't wrote it originally, but from reading the code I think it checks the following:

  • set timestamp that includes mills/micros
  • check that the timestamp millis conversion is greater than the second before and smaller from the second after

There might be a problem either with the timezone or the millis if I have to guess.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dragneelfps

dragneelfps commented Jun 5, 2021

Copy link
Copy Markdown
Contributor Author

I noticed something, postgres is returning timestamps in UTC string, even when I insert it with non UTC offset.
The tests are expecting non UTC offset. So I changed a bit and is actually testing if the two timestamps are equal as OffsetDateTime
Is this correct approach?
I am guessing postgres returning UTC string is related to server settings (which I am not really changing). Is this correct assumption?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@codecov

codecov Bot commented Jun 5, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Codecov Report

Merging #233 (c8313ef) into master (a5a881f) will increase coverage by 0.14%.
The diff coverage is 64.28%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #233      +/-   ##
============================================
+ Coverage     80.03%   80.17%   +0.14%     
- Complexity     1074     1079       +5     
============================================
  Files           265      266       +1     
  Lines          3916     3919       +3     
  Branches        522      523       +1     
============================================
+ Hits           3134     3142       +8     
+ Misses          534      531       -3     
+ Partials        248      246       -2     
Impacted Files Coverage Δ
.../src/main/java/com/github/jasync/sql/db/RowData.kt 84.00% <ø> (ø)
.../com/github/jasync/sql/db/column/SQLTimeEncoder.kt 66.66% <0.00%> (ø)
.../jasync/sql/db/mysql/binary/decoder/DateDecoder.kt 100.00% <ø> (ø)
...ync/sql/db/mysql/binary/encoder/CalendarEncoder.kt 0.00% <0.00%> (ø)
...ync/sql/db/mysql/binary/encoder/DateTimeEncoder.kt 25.00% <0.00%> (ø)
...ync/sql/db/mysql/binary/encoder/JavaDateEncoder.kt 25.00% <0.00%> (ø)
...nc/sql/db/mysql/binary/encoder/LocalTimeEncoder.kt 0.00% <0.00%> (ø)
.../db/mysql/binary/encoder/ReadableInstantEncoder.kt 0.00% <0.00%> (ø)
...sync/sql/db/mysql/binary/encoder/SQLDateEncoder.kt 25.00% <0.00%> (ø)
...sync/sql/db/mysql/binary/encoder/SQLTimeEncoder.kt 25.00% <0.00%> (+5.00%) ⬆️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5a881f...c8313ef. Read the comment docs.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dragneelfps
Switch to JSR 310 Date and Time API
b0aab75
- Removes joda from encoding/decoding
- Using Threeten for PeriodDuration
@dragneelfps
dragneelfps force-pushed the master branch from 51a4f5e to b0aab75 Compare June 5, 2021 18:36
@oshai

oshai commented Jun 5, 2021

Copy link
Copy Markdown
Contributor

I noticed something, postgres is returning timestamps in UTC string, even when I insert it with non UTC offset.
The tests are expecting non UTC offset. So I changed a bit and is actually testing if the two timestamps are equal as OffsetDateTime
Is this correct approach?
I am guessing postgres returning UTC string is related to server settings (which I am not really changing). Is this correct assumption?

According to https://www.postgresql.org/docs/9.1/datatype-datetime.html 8.5.2. Date/Time Output:

The default is the ISO format
and it looks like:

ISO | ISO 8601/SQL standard | 1997-12-17 07:37:16-08

(without time zone)

Conversion on test makes sense (to see it's the same date as inserted). It's just not clear how this was changed from pervious impl.

I still need to review all changes, but it will take some time.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

oshai
oshai reviewed Jun 13, 2021
View reviewed changes

@oshai oshai left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks again for the PR.
I left some comments in code and have some more general things:

  • I saw that in some cases there is no test coverage: do you think it's an issue with the framework, or can you please add tests to cover those cases?
  • I saw in many places move from millis to nano. Is it because java 8 uses nano and joda/db use millis?

About the migration to 2.x: I am thinking if we can add a suggestion to users how to do that more easily? Do you have any suggestion?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread db-async-common/src/main/java/com/github/jasync/sql/db/column/LocalDateTimeEncoderDecoder.kt Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread gradle.properties Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread mysql-async/src/main/java/com/github/jasync/sql/db/mysql/binary/encoder/DateTimeEncoder.kt Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread mysql-async/src/test/java/com/github/jasync/sql/db/mysql/ConnectionHelper.kt
created_at_date DATE not null,
created_at_datetime DATETIME not null,
created_at_timestamp TIMESTAMP not null,
created_at_datetime DATETIME(6) not null,

@oshai oshai Jun 13, 2021

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

why was it changed?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@dragneelfps

dragneelfps commented Jun 13, 2021

Copy link
Copy Markdown
Contributor Author

I saw that in some cases there is no test coverage: do you think it's an issue with the framework, or can you please add tests to cover those cases?

Can you point me to which cases are you talking about specifically?

I saw in many places move from millis to nano. Is it because java 8 uses nano and joda/db use millis?

Yes. Java uses nano, joda uses millis, db micro(at least in mysql)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@oshai

oshai commented Jun 13, 2021

Copy link
Copy Markdown
Contributor

Can you point me to which cases are you talking about specifically?

Look at LocalDateTimeEncoderDecoder:17 and see more examples in 'files changed' with the warning: Added line #Lxx was not covered by tests

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dragneelfps
Fix formatter
7c702c8
@oshai

oshai commented Jun 24, 2021

Copy link
Copy Markdown
Contributor

@dragneelfps before merging there was still one answered question in the review about DATETIME(6).

In addition, will you have some time to improve test coverage?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions

github-actions Bot commented Aug 24, 2021

Copy link
Copy Markdown

Stale pull request message

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot added the no-pr-activity label Aug 24, 2021
@oshai

oshai commented Aug 24, 2021

Copy link
Copy Markdown
Contributor

I will merge this PR soon. Will create 1.x branch before that.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@oshai
Merge branch 'master' into master
c8313ef
@oshai

oshai commented Aug 30, 2021

Copy link
Copy Markdown
Contributor

Thanks for the PR!
I merged it and will release a new version and update docs soon.

maksymilian-gasztych reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@oshai
oshai merged commit 46ab065 into jasync-sql:master Sep 1, 2021
@oshai oshai removed the no-pr-activity label Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oshai oshai oshai left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@dragneelfps @oshai
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.