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

add tests for LONGDATETIME parsing/encoding #631

Merged
merged 1 commit into from
Oct 30, 2023
Merged

Conversation

Connum
Copy link
Contributor

@Connum Connum commented Oct 29, 2023

Description

add tests for parsing and encoding the LONGDATETIME type

Motivation and Context

This was not tested before, and a FIXME comment suggested to do so once it was implemented (which it has been for a long time).

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Testing

Checklist:

  • I did npm run test and all tests passed green (including code styling checks).
  • I have added tests to cover my changes.
  • My change requires a change to the documentation.
  • I have updated the README accordingly.
  • I have read the CONTRIBUTING document.

@Connum Connum added the dev Changes revolving merely around dev-related code like testing, build process, etc. label Oct 29, 2023
Copy link
Contributor

@ILOVEPIE ILOVEPIE left a comment

Choose a reason for hiding this comment

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

This looks good, but we should probably finish implementing LONGDATETIME, or at least add it to the issue tracker. We should also start maintaining a separate issues list on trello or github projects.

@Connum
Copy link
Contributor Author

Connum commented Oct 30, 2023

we should probably finish implementing LONGDATETIME

I already looked into that but we'd need to use BigInt(), which means we would either loose backward compatibility or have to use a library to handle the 64bit timestamps. All that for an issue that we'll face in 15 years from now, where it should be possible to use BigInt() by then.

But yes, we should definitely keep track of these things!

@Connum Connum merged commit 7a1b1ac into master Oct 30, 2023
1 check passed
@Connum Connum deleted the tests/LONGDATETIME branch October 30, 2023 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev Changes revolving merely around dev-related code like testing, build process, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants