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

chore: Change default datetime format #373

Merged
merged 1 commit into from
Dec 17, 2023

Conversation

tienvx
Copy link
Contributor

@tienvx tienvx commented Nov 30, 2023

@tienvx tienvx requested review from JP-Ellis and YOU54F November 30, 2023 03:27
@tienvx tienvx force-pushed the change-default-datetime-format branch 3 times, most recently from 8975cd4 to 2d358b3 Compare December 1, 2023 05:30
@Lewiscowles1986
Copy link
Collaborator

both of your example date formats are non-trivially incorrect.

"2023-12-334T17:15:07", not "2023-11-334T17:15:07" as expected. I'm not sure what is the problem here.

it should surely be 2023-12-30T17:15:07. What is parsing these odd formats? PHP date format is very different from what you have here https://www.php.net/manual/en/datetime.format.php

@Lewiscowles1986
Copy link
Collaborator

this also does not seem to match pact-foundation/docs.pact.io#88 (comment)

@tienvx
Copy link
Contributor Author

tienvx commented Dec 8, 2023

About the format problem in those v3 matchers:

  • The formats in Supported matching rules section of the specification are:
    • yyyy-MM-dd HH:ss:mm
    • yyyy-MM-dd
    • HH:ss:mm
  • The formats in Supported generators section are:
    • YYYY-mm-DD'T'HH:mm:ss
    • yyyy-MM-dd
    • HH:mm::ss

They produce weird value. They are inconsistent. I think some of them are even invalid? I think they are just example values. But I was stupid. I didn't pay attention. I just blindly copied from the Supported generators section. And here we are in the situation of the PR's description.

So I am suggesting to change the default formats to:

  • yyyy-MM-dd'T'HH:mm:ss (or even remove the T character, to become yyyy-MM-dd HH:mm:ss ?)
  • yyyy-MM-dd
  • HH:mm:ss

@tienvx
Copy link
Contributor Author

tienvx commented Dec 8, 2023

Given the information in the above comment

it should surely be 2023-12-30T17:15:07

If you manually set the system's date to 30 November 2023, and then run the test, the month will change from 11 to 12. Note the date 334 not 30 (but nothing wrong with it, it match the format DD, not dd).

What is parsing these odd formats?

I know it's Rust code, but I don't know exactly where it is in the pact-reference code base. I will update when I found it.

PHP date format is very different from what you have here https://www.php.net/manual/en/datetime.format.php

Yes, it's Java date format, not PHP date format

this also does not seem to match pact-foundation/docs.pact.io#88 (comment)

Yes, those are regex-based matchers (specification v2), and I didn't touch them

@Lewiscowles1986
Copy link
Collaborator

Thanks, I really appreciate the consistent efforts you are making; I'm going to recuse myself rather than be unhelpful.

@tienvx tienvx merged commit 14a6235 into pact-foundation:ffi Dec 17, 2023
26 checks passed
@tienvx tienvx deleted the change-default-datetime-format branch December 17, 2023 04:57
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.

2 participants