Ability to clear metadata fields #2216
Conversation
| } | ||
|
|
||
| #[cfg(feature = "clap")] | ||
| pub const KEEP: &str = "::keep"; |
There was a problem hiding this comment.
This (and the Display instance below) is quite ugly, but that's the best I could come up with if we want to keep AddMetadata both the API and the clap interface because of clap-rs/clap#4558
Alternatively we could have a separate struct for clap with option fields that can default to None without going through the value parser. Let me know if you prefer that.
There was a problem hiding this comment.
I agree the behavior here was hard to parse for me, and I don't understand how it would be rendered in the help text either. I don't love the ::keep sigil on the cli. Without knowing clap's facilities for this very well: is there some way to get --<name>=value to have the current set behavior, and expose an additional --clear-<name> as an additional flag?
| } | ||
|
|
||
| #[cfg(feature = "clap")] | ||
| pub const KEEP: &str = "::keep"; |
There was a problem hiding this comment.
I agree the behavior here was hard to parse for me, and I don't understand how it would be rendered in the help text either. I don't love the ::keep sigil on the cli. Without knowing clap's facilities for this very well: is there some way to get --<name>=value to have the current set behavior, and expose an additional --clear-<name> as an additional flag?
I would prefer that command line interface too! (Currently the rendered help is the worst part as it shows these Unfortunately (?) I think it is only possible if we give up that the Are you OK with that? |
|
Yes, please write whatever datastructures Clap requires to provide the ideal cli interface, and then translate the output of clap's parsing to the AddMetadata enums used by the Rust API. You can sequester all of the clap-specific intermediate representations into either a private mod, or a |
There was a problem hiding this comment.
Looks great. Could you please add a quick cli test to cover the clap portion of this as well? for example https://github.com/bytecodealliance/wasm-tools/blob/main/tests/cli/add-metadata-merge-sections.wat
|
Added one |
|
Thanks very much! |
Resolves #2211