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

Activate Chrono's serde feature in extras #961

Conversation

quodlibetor
Copy link
Contributor

AFAICT #rust-lang/cargo/633 doesn't allow only activating a feature.
Activating a downstream feature means that you unconditionally pull it into the
crate.

That means we can't do:

[features]
serde_json = ["chrono/serde"]

because it would always pull chrono in even if it's unused.

This was spurred by #960

AFAICT #rust-lang/cargo/633 doesn't allow _only_ activating a feature.
Activating a downstream feature means that you unconditionally pull it into the
crate.

That means we can't do:

```
[features]
serde_json = ["chrono/serde"]
```

because it would always pull chrono in even if it's unused.
@killercup
Copy link
Member

Thanks! Let's talk a bit more about the use case of this. I think I was a bit quick to suggest this.

Currently, we can convert to/from serde_json::Value and chrono::DateTime. Can the two ever interact in a way that the user can omit chrono's serde feature in their own Cargo.toml? I don't think so. This would mean automatically activating this would be superfluous in some cases, and may actually be confusing in others (features are additive and diesel silently activating this may be confusing in a "why does this even work?" way).

Very interested in your opinions!

@quodlibetor
Copy link
Contributor Author

I'm not actually sure how the mapping from serde_json::Value -> chrono::DateTime works right now, is it mostly the To/FromSql traits? Looking through diesel's chrono code I don't see any specific benefit to enabling the serde feature. It would mostly just enable the #[derive...] calls for upstream, but if users are using chrono then they probably should just enable the feature themselves.

So, yeah, I don't see a a tremendous amount of benefit to auto-enabling the serde feature in chrono if not having it is fine. This PR was largely a "oh, huh, that does work" situation.

@killercup
Copy link
Member

This PR was largely a "oh, huh, that does work" situation.

And that's totally fine 😄

It would mostly just enable the #[derive...] calls for upstream, but if users are using chrono then they probably should just enable the feature themselves.

Yeah I too think that's the only thing happening with the PR. And user's should do that explicitly.

I'll close this PR for now, but feel free to ping me/reopen when you can think of another reason to have this :)

@killercup killercup closed this Jun 22, 2017
@quodlibetor quodlibetor deleted the activate-chrono_serde-in-extras branch June 22, 2017 14:33
@quodlibetor
Copy link
Contributor Author

I agree with that decision 👍

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