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

Add serde derives for some config datastructures #480

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

xermicus
Copy link
Contributor

@xermicus xermicus commented Feb 27, 2024

Description

Add serde derives for some config datastructures. Otherwise they can not be reused when deriving the compilers configuration (or it has to be implemented manually).

Hidden behind serde feature flag so it should be pretty non-invasive.

Related Issue

How This Has Been Tested

Checklist

@xermicus
Copy link
Contributor Author

xermicus commented Mar 7, 2024

@TheDan64 you assigned this to me - is there anything left for me to get this merged?

@TheDan64
Copy link
Owner

TheDan64 commented Mar 7, 2024

Apologies, I am traveling this month and might not have time to review for a few weeks

@xermicus
Copy link
Contributor Author

xermicus commented Mar 7, 2024

No worries, enjoy!

@TheDan64 TheDan64 self-requested a review March 11, 2024 03:12
@TheDan64
Copy link
Owner

Looks good; but I'd like to just call the feature serde rather than serde-derive as this seems more typical (apparently just making the crate optional is an automatic feature?):
https://github.com/uuid-rs/uuid/blob/8c6fc53484e8a374b01d71f311091c8d21893dc3/Cargo.toml#L40
https://github.com/servo/rust-smallvec/blob/55229397eb97a7ced88383182260999fc81c2393/Cargo.toml#L22
https://github.com/indexmap-rs/indexmap/blob/184fe4bfcba20e21242c924220170b98be157d45/Cargo.toml#L21

@xermicus
Copy link
Contributor Author

@TheDan64 Yeah I definitively agree. Changed so we just have the serde feature

Copy link
Owner

@TheDan64 TheDan64 left a comment

Choose a reason for hiding this comment

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

Thanks!

@TheDan64 TheDan64 merged commit 69c5a3f into TheDan64:master Mar 12, 2024
16 checks passed
@xermicus xermicus deleted the serde-derive branch March 12, 2024 09:06
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