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: replace chrono with time #72

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SimenB
Copy link

@SimenB SimenB commented Nov 27, 2023

About the changes

We've replaced all usage of chrono with time (which chrono originally was designed to supersede) at work - pulling in unleash-client bloated our lockfiles again 😅

We find the API easier to use, and the lowered compile times/fewer dependencies is an added bonus

N/A

Important files

N/A

Discussion points

Does this need to be feature gated? I guess it does since the structs have pub fields containing instances.

But in case you're not interested in this change, I thought to open up a PR early.

@FredrikOseberg
Copy link

Thanks @SimenB! I'm raising this internally and we'll have a look at this PR. I'll get back to you.

Copy link
Member

@sighphyre sighphyre left a comment

Choose a reason for hiding this comment

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

I don't think I have strong feelings here, but this is a public field and probably does need a feature gate and the tests need to pass.

@rbtcollins Anything about this that worries you?

@sjaanus
Copy link
Contributor

sjaanus commented Jan 16, 2024

@rbtcollins Just a quick note to check if you've had a chance to look at the implementation. Your input would be really valuable. Let us know if you need any updates or have questions.

@rbtcollins rbtcollins changed the title chore: replace chrono with time feat: replace chrono with time Jan 16, 2024
@rbtcollins
Copy link
Collaborator

Thanks for the ping! Grabbing my via the unleash slack might be a tad more reliable... I get a lot of github notifications :/

Ok so first thing, code level review. This is a breaking change as it changes the type of a public field. So the change should come in with a semver-major version bump. e.g. 0.11.0. Codewise the change seems straight forward.

I'm not aware of enough the current ecosystem consensus I guess - but just poking around reddit etc it seems both chrono and time are currently maintained.

We could:

  • say no
  • say yes
  • make the struct parameterised by a local time trait, and use a local interface, so that people can choose their favorite time implementation.

None of the actual apis take or produce time - it is all internal concerns.

@sjaanus / @sighphyre does Unleash have a view on chrono vs time? If you do, I'm happy to follow that. E.g. if you're using chrono, we should probably do the local trait thing, like we did for async HTTP clients. But if you're very time centric, then maybe just switching over is better?

@sighphyre
Copy link
Member

Thanks for the inputs @rbtcollins!

does Unleash have a view on chrono vs time?

Not really, to be honest, which is why we were wondering if you did! We use chrono pretty consistently throughout our other Rust projects, it's worked pretty well for us, but that's not an active decision to reject time.

I'm leaning towards either saying no to this or accepting a version that does the local trait if @SimenB is willing to take a look (assuming this isn't a maintenance burden).

@rbtcollins
Copy link
Collaborator

A local trait will be low maintenance

@sighphyre
Copy link
Member

@SimenB Sounds like we're all happy for the ball to be in your court. Would you be interested in pushing this forward or shall I close it?

@SimenB
Copy link
Author

SimenB commented Jan 24, 2024

sweet 👍 happy to give it a whack, but I cannot promise any timeline. Hopefully within the next couple of weeks 🙂

@sighphyre
Copy link
Member

@SimenB Wonderful! No timeline needed, when you have time and are interested in pushing this forward

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

5 participants