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

feat: Add serde feature #65

Closed
wants to merge 2 commits into from
Closed

feat: Add serde feature #65

wants to merge 2 commits into from

Conversation

nerdo
Copy link

@nerdo nerdo commented May 10, 2023

Optionally allow verbosity flag to be serialized with serde.

Useful for when you want to easily save and load a configuration.

Optionally allow verbosity flag to be serialized with serde.

Useful for when you want to easily save and load a configuration
Copy link
Contributor

@matthiasbeyer matthiasbeyer left a comment

Choose a reason for hiding this comment

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

Hi! Thanks for your changes!

I think they can go in, but I'd like to see the one thing changed I annotated. No need to duplicate the #[cfg] attribute here 😆 I hope that's okay for you!

src/lib.rs Outdated Show resolved Hide resolved
@epage
Copy link
Member

epage commented May 12, 2023

I think they can go in, but I'd like to see the one thing changed I annotated. No need to duplicate the #[cfg] attribute here 😆 I hope that's okay for you!

I disagree. I think more of a case is needed for why a CLI flag needs serialization, particularly the fact that this was done in a raw way (separate verbose, quiet as u8s)

@nerdo
Copy link
Author

nerdo commented May 13, 2023

I think they can go in, but I'd like to see the one thing changed I annotated. No need to duplicate the #[cfg] attribute here 😆 I hope that's okay for you!

I disagree. I think more of a case is needed for why a CLI flag needs serialization, particularly the fact that this was done in a raw way (separate verbose, quiet as u8s)

I'm not sure what other use cases there are for serializing cli flags, but I think the clearest use case for me is the one I mentioned: saving and loading cli configurations.

That is what I'm using it for in my cli app. I proposed doing it as an opt-in feature with serde which is pretty ubiquitous.

The fact that they are u8's is a valid question. I would prefer it if at least quiet were boolean, but I didn't want scope creep - I just wanted to focus on being able to load and save configs and I found that when I added this crate, I could no longer do that.

Co-authored-by: Matthias Beyer <[email protected]>
@schwa
Copy link

schwa commented Aug 11, 2023

I've been hoping to see this change so I can separate out my config from my args.

Progress seems stalled?

@epage
Copy link
Member

epage commented Aug 12, 2023

More design discussion is needed on this because exposing separate verbose and quiet fields that are u8 does not make sense.

As per my usual, I'd prefer those problem/solution discussions to happen on issues, keeping PRs focused on the implementation. Closing this until we have a design worked out. Feel free to create an issue.

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.

4 participants