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 support for jiff #526

Merged
merged 2 commits into from
Aug 16, 2024
Merged

Add support for jiff #526

merged 2 commits into from
Aug 16, 2024

Conversation

snaggen
Copy link
Contributor

@snaggen snaggen commented Aug 13, 2024

I have added basic support for the new jiff datetime library.

@tiagolobocastro
Copy link
Collaborator

On their README, they have:

My plan is to iterate on the Jiff API and make occasional breaking change releases over the next ~year.

Though considering we don't actually depend on the api, only on the type names, we should remain relatively unaffected?

@snaggen
Copy link
Contributor Author

snaggen commented Aug 13, 2024

Yes, it is a fairly new create, but this is only for the basic types, and their serialization is most likely quite stable. And in the unlikely event that they break, then we just need to update. Users of jiff are aware that it is unstable, so it should be OK.

@BurntSushi
Copy link

One possibility is to use a jiff01 feature instead. That way, if there's a jiff 0.2, then you can add a jiff02 feature and so on. But it looks like you aren't doing this for other libraries, so I assume you're doing semver incompatible releases whenever a public dependency gets a semver incompatible release?

But yes, I do expect the serialization format to be stable. Everything conforms to the Temporal ISO 8601 spec (which combines ISO 8601, RFC 3339 and RFC 9557).

@tiagolobocastro
Copy link
Collaborator

We do have multiple features for a couple of crates, at least for uuid IIRC.
I can't remember if there's ever been any incompatibilities to be honest, we've either been lucky or they're not reported.

Maybe still we should jiff01 for now just in case. What do you think @snaggen?

@snaggen
Copy link
Contributor Author

snaggen commented Aug 14, 2024

The only serious breakage I see is if the datatype names is changed, and it seems quite unlikely for that to happen.
If the serialization format changes, it will still not affect much, other that the string format hint will be inaccurate, but the datatype string is most likely still correct.

So, unless we think the basic datatypes, Timestamp, Zoned, Span, civil::Date, civil::Time will change names, I think we should be fine using "jiff" as the feature.

@BurntSushi
Copy link

I confess I don't understand the focus on names here. If jiff 0.2 is published without any changes to names, it is still true that the 0.1 version of jiff::Timestamp is a distinct type from the 0.2 version of jiff::Timestamp.

To me, the question is, what will you do when chrono (for example) publishes a 0.5 release? Would you add a chrono05 feature? Publish a breaking change? Wish you had added a chrono04 feature initially?

@snaggen
Copy link
Contributor Author

snaggen commented Aug 14, 2024

What I mean is that we only do a simple mapping, that will annotate the type
jiff::Timestamp as a String with hint DateTime. As long as there is a struct jiff::Timestamp things will still compile, and as long as that Struct serializes to a String we are also correct. If the type is not a valid timestamp, well then the hint DateTime is not correct, but that is a minor thing.

So, that is why I say that the only way we risk breakage is if the Structs change names, since that will prevent compilation.

So, this is why I don't think it is worth to use "jiff01" since I think that there are not really much risk of breakage since we have a very basic support.

@BurntSushi
Copy link

Hmmm, okay. I think I probably don't have the context for this project to make a suggestion then. If Jiff types don't appear in the public API of this crate, then I don't think my concerns apply.

@snaggen
Copy link
Contributor Author

snaggen commented Aug 14, 2024

Yes, the details of the jiff datatypes are not exposed or used here in any way, the only thing this patch does is to add these basic annotations for the types Timestamp, Zoned, Span, civil::Date, civil::Time. So you may change the semantics of the types, add remove methods and this will not be affected. As long as the Structs still exists and serialize to String, we are ok.

@tiagolobocastro
Copy link
Collaborator

To me, the question is, what will you do when chrono (for example) publishes a 0.5 release? Would you add a chrono05 feature? Publish a breaking change? Wish you had added a chrono04 feature initially?

If the type names don't change, then maybe we could cover both 0.4 and 0.5 within the same feature name?

Hmmm, okay. I think I probably don't have the context for this project to make a suggestion then. If Jiff types don't appear in the public API of this crate, then I don't think my concerns apply.

Basically this crates has macros which go through http handler signature and parse them to generate an openapi spec.
In order to do this we have to know how a given type can be represented in openapi.
For this we have traits (ex: TypedData) and macros which we use to implement the traits, example:

macro_rules! impl_type_simple {
    ($ty:ty, $dt:expr, $df:expr) => {
        impl TypedData for $ty {
            fn data_type() -> DataType {
                $dt
            }
            fn format() -> Option<DataTypeFormat> {
                Some($df)
            }
        }
    };
}

impl_type_simple!(jiff::Timestamp, DataType::String, DataTypeFormat::DateTime);

And now if an http handler mentions the type jiff::Timestamp, then we can generate it as an openapi string data type with the DateTime format, example:

#[api_v2_operation]
    async fn get_timestamp() -> Result<web::Json<jiff::Timestamp>, actix_web::Error> {
        ...
    }

And this is why we add these features, so we can map external crates types to openapi types.
Another way of doing is can sometimes can be to use newtype pattern and implementing the traits yourself. Ofcourse this is not always possible.

@BurntSushi
Copy link

Gotya okay. If the Jiff data types aren't actually appearing in the API of your crate, and this really is just type names, then I agree this is probably fine because it means Jiff isn't actually a public dependency.

There is a chance that Jiff's civil module gets renamed to plain in the future though, so it would then end up being plain::DateTime instead of civil::DateTime.

@snaggen
Copy link
Contributor Author

snaggen commented Aug 16, 2024

Gotya okay. If the Jiff data types aren't actually appearing in the API of your crate, and this really is just type names, then I agree this is probably fine because it means Jiff isn't actually a public dependency.

There is a chance that Jiff's civil module gets renamed to plain in the future though, so it would then end up being plain::DateTime instead of civil::DateTime.

Ok, that would cause this to break, so maybe we go with jiff01, jiff02 until the API is stable, I will push an update

Btw, I'm all for the civil to plain change, for me as a non-native english speaker civil feels a bit strange, while plain is a lot more natural.

@tiagolobocastro
Copy link
Collaborator

Alright let's go with jiff01 for now, thank you both!

@tiagolobocastro tiagolobocastro merged commit 972ef4f into paperclip-rs:master Aug 16, 2024
6 checks passed
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