search

Unleash / unleash-client-rust

Unleash client SDK for Rust language projects
Apache License 2.0
23 stars 18 forks source link

feat: replace `chrono` with `time` #72

Open SimenB opened 9 months ago

SimenB commented 9 months ago

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 commented 9 months ago

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

sjaanus commented 8 months ago

@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 commented 8 months ago

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:

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 commented 8 months ago

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 commented 8 months ago

A local trait will be low maintenance

sighphyre commented 7 months ago

@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 commented 7 months ago

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

sighphyre commented 7 months ago

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