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

Cleanups #661

Merged
merged 18 commits into from
Mar 22, 2022
Merged

Cleanups #661

merged 18 commits into from
Mar 22, 2022

Conversation

djc
Copy link
Member

@djc djc commented Mar 22, 2022

My goal here is to make the codebase easier to navigate and take advantage of the MSRV upgrade from #653 to employ more modern Rust standards.

@djc djc force-pushed the cleanups branch 3 times, most recently from b8b4694 to 013507c Compare March 22, 2022 04:21
use crate::offset::{FixedOffset, LocalResult, TimeZone, Utc};
use core::fmt;
use core::ops::Deref;
use rustc_serialize::{Decodable, Decoder, Encodable, Encoder};
Copy link
Member

Choose a reason for hiding this comment

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

Why use rustc_serialize? Serde?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not making any backwards incompatible changes here, so that means keeping rustc_serialize.

Copy link
Member

@Milo123459 Milo123459 left a comment

Choose a reason for hiding this comment

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

LGTM, although serde should be the default.

@Milo123459 Milo123459 merged commit d6d02ac into main Mar 22, 2022
@Milo123459 Milo123459 deleted the cleanups branch March 22, 2022 07:18
@djc
Copy link
Member Author

djc commented Mar 23, 2022

LGTM, although serde should be the default.

Do you mean that we should enable the serde feature by default? I don't agree -- most well-maintained libraries keep it as an off-by-default feature, so that downstream users who don't need it don't have to compile the extra dependency.

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.

2 participants