Skip to content
This repository has been archived by the owner on Jun 2, 2024. It is now read-only.

Disable time features #333

Merged
merged 2 commits into from
Jan 30, 2023
Merged

Disable time features #333

merged 2 commits into from
Jan 30, 2023

Conversation

lnicola
Copy link
Contributor

@lnicola lnicola commented Jan 3, 2023

I left these enabled in dev-dependencies, so that the tests still work, but to be honest, I don't think those tests are really useful.

The reason to disable this is the proc macro stuff has a significant build time impact, and it's not really something zip users need.

@zamazan4ik
Copy link
Contributor

@Plecra we need your opinion here.

Copy link
Member

@Plecra Plecra left a comment

Choose a reason for hiding this comment

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

Argh, more compatibility hazards. Fine, it's worth it. Hopefully we won't run into compatibility hazards, though I'd like to use a changelog to help users upgrading to the new version.

@Plecra
Copy link
Member

Plecra commented Jan 30, 2023

Hmm, I'm not sure about standard ways to to this, but I can pop a CHANGELOG.md in the root.

@Plecra
Copy link
Member

Plecra commented Jan 30, 2023

Oops! We've got a bug in the fuzz CI. You'll need to add "std" to the featureset for time, otherwise we can't get the current time.

@Plecra
Copy link
Member

Plecra commented Jan 30, 2023

(That's https://docs.rs/time/latest/time/struct.OffsetDateTime.html#method.now_utc btw - it mentions the feature dep)

@lnicola
Copy link
Contributor Author

lnicola commented Jan 30, 2023

Done, can you try again? The clippy failures are unrelated. I can file a PR to fix them tomorrow, if you want.

@Plecra
Copy link
Member

Plecra commented Jan 30, 2023

We need it for release dependencies too :) the now_utc API is used for writing the current time in file metadata.

@lnicola
Copy link
Contributor Author

lnicola commented Jan 30, 2023

Ah, okay. Fixed.

Copy link
Member

@Plecra Plecra left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks a bunch

@Plecra Plecra merged commit be73175 into zip-rs:master Jan 30, 2023
@lnicola lnicola deleted the time-features branch January 30, 2023 19:29
@lnicola
Copy link
Contributor Author

lnicola commented Jan 30, 2023

Thanks! No pressure, but do you think you'll cut out a new release soon?

@Plecra
Copy link
Member

Plecra commented Jan 31, 2023

Just after addressing the CVE issue, yep!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants