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.

Sign up for GitHub

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

Jump to bottom

Add serde support to concrete units #77

Merged
merged 4 commits into from
Apr 28, 2021
Merged

Add serde support to concrete units #77

merged 4 commits into from
Apr 28, 2021

Conversation

mattico
Copy link
Contributor

@mattico mattico commented Oct 7, 2020

Fixes #55

@mattico
Copy link
Contributor Author

mattico commented Oct 7, 2020

The CI failure is unrelated: rust-lang/rust#77638

@mattico
Copy link
Contributor Author

mattico commented Oct 7, 2020

Includes #78 to get CI working on nightly.

@PTaylor-us
Copy link
Member

Thanks for this! I'll get this reviewed and pulled.

@PTaylor-us PTaylor-us requested a review from korken89 March 19, 2021 14:32
korken89
korken89 approved these changes Mar 20, 2021
View reviewed changes
Copy link
Collaborator

@korken89 korken89 left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks for the effort!

@korken89
Copy link
Collaborator

Ping @PTaylor-us

@PTaylor-us
Copy link
Member

PTaylor-us commented Apr 27, 2021 •
edited
Loading

@korken89 @mattico Is there a downside to not putting it behind a feature flag?

@mattico
Enable resolver 2 to fix std/no_std dependencies
c2b1700 
This was stabilized in Cargo 1.51. Leaving the example crate for now
to support older stable versions.
@mattico
Copy link
Contributor Author

mattico commented Apr 27, 2021

Enabling the serde support increases the number of built crates from 16 to 30 and doubles the clean build time on my 16-thread machine from 9s to 18s.

We could enable the feature by default but it is nice to provide the option to disable it for people who care about such things.

@korken89
Copy link
Collaborator

It's common practice to have serde support behind a flag, as it is not the main aim of the library. Plus compile time bloat as serde is a beast :)

@PTaylor-us
Copy link
Member

Thank you for this!

@PTaylor-us PTaylor-us merged commit fd63640 into FluenTech:master Apr 28, 2021
@mattico mattico deleted the serde branch April 29, 2021 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@korken89 korken89 korken89 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add serde Serialize/Deserialize impls to types?
3 participants
@mattico @PTaylor-us @korken89