Skip to content

Add DateTime support to navi#11

Merged
lispyclouds merged 8 commits into
lispyclouds:mainfrom
taxonomy-man:main
Jan 24, 2025
Merged

Add DateTime support to navi#11
lispyclouds merged 8 commits into
lispyclouds:mainfrom
taxonomy-man:main

Conversation

@taxonomy-man
Copy link
Copy Markdown
Contributor

I think Datetime OpenApi object corresponds to inst? Malli validation semantics.

@lispyclouds
Copy link
Copy Markdown
Owner

Thanks for this, taking a look soon!

@lispyclouds
Copy link
Copy Markdown
Owner

Can it be tested a bit more to see what the result of the parse is from the java lib? Not sure if that's the one that malli expects.

Comment thread test/navi/impl_test.clj Outdated
Comment thread test/navi/impl_test.clj Outdated
Comment thread test/navi/impl_test.clj
@lispyclouds
Copy link
Copy Markdown
Owner

lispyclouds commented Jan 20, 2025

More notes:

There are two types of temporal value strings:

  • DateSchema: returns the value parsed as a java.util.Date satisfying inst?. This is when the string format is date.
  • DateTimeSchema: returns the value parsed as a java.time.OffsetDateTime which doesn't satisfy inst?. This is when the string format is date-time.

We should be handling both the cases when transforming. When being run in a server we would be getting the values as strings; we need to check if reitit coerces them into the correct types before malli checks them.

@lispyclouds
Copy link
Copy Markdown
Owner

lispyclouds commented Jan 21, 2025

@taxonomy-man Just verified that inst? should handle both. If we add the support for DateSchema as well here and clean it up a bit, we should be good to merge!

@taxonomy-man
Copy link
Copy Markdown
Contributor Author

ok that sounds great! haha yes, Im unfortunately blind to whitespaces. Ok then I will have a second look 👍

Comment thread newopenapispec.yaml Outdated
Comment thread src/navi/core.clj Outdated
@taxonomy-man
Copy link
Copy Markdown
Contributor Author

@lispyclouds now I have cleaned up stuff and added tests

Comment thread test/navi/transform_test.clj
@taxonomy-man
Copy link
Copy Markdown
Contributor Author

👍

@lispyclouds
Copy link
Copy Markdown
Owner

Looks good, will release with the changes in #13 and #14
Thanks!

@lispyclouds lispyclouds merged commit 05d9c6e into lispyclouds:main Jan 24, 2025
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