Skip to content

[mysql] Stop truncating timestamp milliseconds#668

Merged
AndriiSherman merged 4 commits intodrizzle-team:mainfrom
steviec:main
Aug 9, 2023
Merged

[mysql] Stop truncating timestamp milliseconds#668
AndriiSherman merged 4 commits intodrizzle-team:mainfrom
steviec:main

Conversation

@steviec
Copy link
Copy Markdown
Contributor

@steviec steviec commented May 30, 2023

The mysql Timestamp column's mapToDriverValue method was incorrectly truncating everything after the seconds when serializing Date objects to strings. This commit preserves the milliseconds. Note that this only applies to {mode: 'date'}; the string mode doesn't seem to have this same problem.

I noticed that there's a variety of other bugs related to time precision in the mysql adapter. I'll continue to look at ways to improve it, but I wanted to at least get this fix in.

Copy link
Copy Markdown
Contributor

@dankochetov dankochetov 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 the contribution!
We require all commits to our codebase to be signed. Please sign them and force-push.

@steviec
Copy link
Copy Markdown
Contributor Author

steviec commented Jun 5, 2023

Whoops, sorry about that! I rebased, amended my commit with the signed version, and force pushed. Let me know if there's anything else I need to do.

Copy link
Copy Markdown
Contributor

@dankochetov dankochetov left a comment

Choose a reason for hiding this comment

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

Please use dprint to format the code you've changed.

steviec added 2 commits June 8, 2023 06:29
The Mysql timestamp was incorrectly truncating everything after the seconds
when serializing Date objects to strings. This commit preserves the milliseconds.
@steviec
Copy link
Copy Markdown
Contributor Author

steviec commented Jun 8, 2023

Done.

Has your team talked about making dprint linting integrated more directly into the repo? E.g. adding a script in package.json, or making it a pre-commit hook to ensure that it's happening? I'd be happy to add that, but didn't know if you were intentionally not doing it for some reason.

@steviec
Copy link
Copy Markdown
Contributor Author

steviec commented Jun 19, 2023

Hi @dankochetov, could I get this merged in? It would be great to not have to continue using the forked version and having to keep it up-to-date with the upstream.

@AndriiSherman AndriiSherman dismissed dankochetov’s stale review August 9, 2023 13:03

This one is fixed. I don't know how to close review in some other way

@AndriiSherman AndriiSherman merged commit 4db1e9f into drizzle-team:main Aug 9, 2023
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