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

remove cfg_attr for serde feature #273

Closed
ralexstokes opened this issue Sep 30, 2023 · 2 comments · Fixed by #274
Closed

remove cfg_attr for serde feature #273

ralexstokes opened this issue Sep 30, 2023 · 2 comments · Fixed by #274

Comments

@ralexstokes
Copy link
Owner

I had originally intended to make the serde feature optional, and gated the derive(serde::{Serialize,Deserialize}) attributes behind it, like this

#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]

But it turns out this conditional attribute would also need to apply to each serde directive as well, like this

#[serde(with = "crate::serde::as_string")]

otherwise, the crate does not compile without the serde feature. cf. #191 and #167

in lieu of adding a lot of code to what is already a bit of a noisy codebase (with all the const generics), I think at the moment I favor just ungating the derive(serde::*) directives for now and just require serde

while it does mean pulling in more crates than could otherwise be possible, serde is a common one and is usually in the closure of whatever crates a consumer of this library would already be using, so for most use cases it wouldn't matter. and it doesn't stop us from supporting no-std as serde has a capability for this as well.

so this issue will be closed once we have made the edits to remove all cfg_attr(serde) attributes.

@leruaa
Copy link
Contributor

leruaa commented Oct 1, 2023

Hi, I can take this on :)

@ralexstokes
Copy link
Owner Author

fixed in #274

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 a pull request may close this issue.

2 participants