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

Replace chrono with time #1304

Closed
uklotzde opened this issue Mar 7, 2022 · 10 comments · Fixed by #1307
Closed

Replace chrono with time #1304

uklotzde opened this issue Mar 7, 2022 · 10 comments · Fixed by #1307

Comments

@uklotzde
Copy link
Contributor

uklotzde commented Mar 7, 2022

chrono seems to be unmaintained and depends on an outdated version of time. For all features required by the engine and the API time should suffice.

As chrono types and modules are re-exported by the public API this will be a breaking change. I also suggest to use this opportunity for removing those re-exports from the API which IMHO do not provide any value and are more confusing than helpful. Explicit, external dependencies are fine and more transparent.

@fulmicoton
Copy link
Collaborator

Thanks. That makes sense. Does somewant to pick this ticket?

@uklotzde
Copy link
Contributor Author

uklotzde commented Mar 9, 2022

Let me check how much effort this takes. I just did it for some of our projects. No promises, but I'll report back soon ;)

@uklotzde
Copy link
Contributor Author

uklotzde commented Mar 9, 2022

I'll take care of this issue.

@fulmicoton
Copy link
Collaborator

Let's not remove the re-exports. They are useful to avoid dependency hell.

First of all, ideally we should not rely on any symbol on the API that is coming from another library.
time::DateTime seems like a valid exception: it is widely used type that have rich operations associated to it.
Making a simplified struct wrapping DateTime for instance could get in the way of the user with no version conflict.

Now let's say two library do the same thing and do not do reexports.
Project P relies on lib A and lib B. lib A and lib B have incompatible constraint on the version of date, how will P interact with lib A and lib B?
With reexport project P have a clear path to solve this problem.

@fulmicoton
Copy link
Collaborator

Tweeted it to see if smarter people can change my mind :)
https://twitter.com/fulmicoton/status/1501733827081023491

@uklotzde
Copy link
Contributor Author

Re-export: You can't use aliased modules/types without knowing where they are actually originating. The re-export alias requires me to think about different import statements, i.e. time:: vs. tantivy::time. And the re-export is even more verbose.

@uklotzde
Copy link
Contributor Author

Now let's say two library do the same thing and do not do reexports. Project P relies on lib A and lib B. lib A and lib B have incompatible constraint on the version of date, how will P interact with lib A and lib B? With reexport project P have a clear path to solve this problem.

I am not aware how a re-export could resolve such a version conflict? You will still end up with unrelated, incompatible types. Visibility and versioning are orthogonal.

@fulmicoton
Copy link
Collaborator

With two conflicting dependency, you will end up building your program with the two different crates.
The reexport let you pick which one you need depending on the context. You can also as a user manually write a function to convert one from the other.

Re-export: You can't use aliased modules/types without knowing where they are actually originating. The re-export alias requires me to think about different import statements, i.e. time:: vs. tantivy::time. And the re-export is even more verbose.

That's not correct. If you don't have a version conflict, you can use your type and ignore the reexport.

@uklotzde
Copy link
Contributor Author

Thanks for the clarification. So the re-export of the time crate as a module would make the incompatible types accessible in case of a version conflict and allow users to manually resolve it when required.

@fulmicoton
Copy link
Collaborator

Yes. I encounter the problem with third part library around once a year :). When it happens it is really painful.

fulmicoton pushed a commit that referenced this issue Mar 21, 2022
For date values `chrono` has been replaced with `time` 
- The `time` crate is re-exported as `tantivy::time` instead of `tantivy::chrono`.
- The type alias `tantivy::DateTime` has been removed.
- `Value::Date` wraps `time::PrimitiveDateTime` without time zone information.
- Internally date/time values are stored as seconds since UNIX epoch in UTC.
- Converting a `time::OffsetDateTime` to `Value::Date` implicitly converts the value into UTC.
If this is not desired do the time zone conversion yourself and use `time::PrimitiveDateTime`
directly instead.

Closes #1304
PSeitz pushed a commit that referenced this issue May 3, 2022
For date values `chrono` has been replaced with `time` 
- The `time` crate is re-exported as `tantivy::time` instead of `tantivy::chrono`.
- The type alias `tantivy::DateTime` has been removed.
- `Value::Date` wraps `time::PrimitiveDateTime` without time zone information.
- Internally date/time values are stored as seconds since UNIX epoch in UTC.
- Converting a `time::OffsetDateTime` to `Value::Date` implicitly converts the value into UTC.
If this is not desired do the time zone conversion yourself and use `time::PrimitiveDateTime`
directly instead.

Closes #1304
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants