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

feat(gen): support custom time format #1334

Merged
merged 2 commits into from
Oct 25, 2024
Merged

Conversation

abemedia
Copy link
Contributor

This PR adds support for a new extension x-ogen-time-format which sets a format for encoding and decoding time values.
Also fixes some indentation on templates (mix of tabs and spaces).

Since I'm not that familiar with the codebase, let me know if I should do some things differently.

Closes #1063

Copy link

codecov bot commented Oct 24, 2024

Codecov Report

Attention: Patch coverage is 87.75510% with 6 lines in your changes missing coverage. Please review.

Project coverage is 71.44%. Comparing base (ce47f75) to head (dbd90b0).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
json/time.go 92.30% 1 Missing and 2 partials ⚠️
jsonschema/parser.go 25.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1334      +/-   ##
==========================================
- Coverage   71.45%   71.44%   -0.01%     
==========================================
  Files         190      190              
  Lines       15584    15587       +3     
==========================================
+ Hits        11135    11136       +1     
  Misses       3898     3898              
- Partials      551      553       +2     

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

json/time.go Outdated Show resolved Hide resolved
Copy link
Member

@tdakkota tdakkota left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

Please move generated code changes to a separate commit.

Also, it would be nice to have a encode-decode-encode test for this feature, like one of these.

Everything else LGTM.

json/time.go Outdated Show resolved Hide resolved
@abemedia abemedia force-pushed the feat/time-format branch 4 times, most recently from d015940 to 7be7d2b Compare October 25, 2024 00:52
@abemedia
Copy link
Contributor Author

abemedia commented Oct 25, 2024

@tdakkota Weirdly when testing on my Intel MacBook I can reproduce similar results as you on the benchmarks. On my M1 Format was actually faster (however the function you wrote in your comments beat it by a little).

Since we're leaving the json package functions I had removed, I changed TimeFormat to only return custom formats so the output of the template no longer changes if no custom format was set.

Also, I had forgotten to add support for parameters so I added that too.

Let me know if there's anything else.

@tdakkota tdakkota merged commit 7e342ba into ogen-go:main Oct 25, 2024
14 checks passed
@abemedia abemedia deleted the feat/time-format branch October 25, 2024 08:06
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.

format: support formatting time.Time as RFC3339Nano
2 participants