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

Deprecate Yaml API #3087

Closed
epage opened this issue Dec 8, 2021 · 25 comments
Closed

Deprecate Yaml API #3087

epage opened this issue Dec 8, 2021 · 25 comments
Milestone

Comments

@epage
Copy link
Member

epage commented Dec 8, 2021

The YAML API is used for:

  • Fast iteration by changing the CLI without re-compiling
  • Allowing compsable CLI pieces, like the derive's flatten
  • Allowing end-user customization of the CLI

However, this API

  • Defers errors from compile time to runtime. For some users, this has cost a lot of time and money during critical parts because of mismatches between their file and the code
  • We can't leverage any of the compile time deprecation messages to help users migrate
  • I have a strong suspicion there are several bugs in it due to our lack of attention to testing this to the same degree as the builder and derive APIs
  • There is a constant impact on maintaining clap to make sure all of our changes are exposed in YAML correctly with sufficient documentation. On top of this, the existing documentation was inline with the builder API, adding extra noise for a builder user to have to sift through to get what they needed.
  • Has few enough users, I suspect who would be burdened by switching to one of the other APIs that it doesn't justify taking resources away from moving clap forward.

If users want to keep the YAML API alive, a suggested path forward

  • Fork YAML support in clap3 as clap_yaml and release it as a 1.0
  • Pull in changes that were removed from clap3, like
  • Release a 2.0

See also https://github.com/aobatact/clap-serde

@epage epage added this to the 3.0 milestone Dec 8, 2021
@epage
Copy link
Member Author

epage commented Dec 8, 2021

Deprecated in 6ce9714

@ilyvion
Copy link

ilyvion commented Dec 9, 2021

Why do all the comments in the code say "deprecated in issue #9" when that issue has nothing to do with YAML or deprecation?

@epage
Copy link
Member Author

epage commented Dec 9, 2021

Why do all the comments in the code say "deprecated in issue #9" when that issue has nothing to do with YAML or deprecation?

I missed updating those references. There was work being done in another org for a short period of time. I'll go update those.

epage added a commit to epage/clap that referenced this issue Dec 9, 2021
These exist pretty much just for YAML (clap-rs#3087).  If anyone else is
building on these, it has a limited shelf-life anyways because of clap-rs#2717.

BREAKING CHANGE: `FromStr` for settings requires the `yaml` feature.
@HitomiTenshi
Copy link

Hi, I'm an avid YAML user and am really disappointed that YAML is getting deprecated! ☹️ I would be glad if it could be continued as a separate crate like you suggested @epage

The YAML configuration positives outweigh the negatives for me greatly:

  • Ease of use and fast iterations during development
  • Clean separation of clap code and actual application logic. (Very important for me!)
  • The YAML configuration file makes it look like a clean description of all the commands the app provides, while at the same time registering all these commands in one go!

I'm all for easy to use, clean and readable code and this is what the YAML configuration provided for me.

Yes, there are a bunch of downsides, but I'm fine with them for the above reasons.

Even though I'm just one developer that uses clap in my own projects (e. g. Dekinai) I hope that you will reconsider supporting YAML in future clap versions. 👍

@epage
Copy link
Member Author

epage commented Dec 12, 2021

Ease of use and fast iterations during development

Are you loading the yaml file at compile time or runtime? The latter offers the faster iteration while the former keeps single-file binaries. Just wanting to better understand how much faster iteration you are getting.

Clean separation of clap code and actual application logic. (Very important for me!)
The YAML configuration file makes it look like a clean description of all the commands the app provides, while at the same time registering all these commands in one go!

What are your thoughts on the Derive API? I feel like it similarly accomplishes these.

I am interested in hearing more about in which ways you appreciate yaml helping separate clap / app logic and why it is important. I can make guesses as to what you are referring to but would rather hear what you are actually aiming for than living with assumptions :).`

I hope that you will reconsider supporting YAML in future clap versions

Would you be willing to help maintain a clap_yaml or clap_serde crate?

@HitomiTenshi
Copy link

HitomiTenshi commented Dec 12, 2021

Just wanting to better understand how much faster iteration you are getting.

With fast iterations I actually meant being able to iterate through changes (try out changes) very quickly during development. I'm not too concerned about any performance hits that clap would cause, because these would only affect application start time. I'm using the load_yaml! macro, which loads the yaml file during compile time if I understood correctly. Compilations are so fast (even with load_yaml's compile time increase), that I didn't have any complaints during development or in production.

What are your thoughts on the Derive API? I feel like it similarly accomplishes these.

I've looked through this documentation and also some examples and I would not use it. My goal is to not clutter .rs files with too much "boilerplate logic", and anything related to CLI commands / arguments / help falls for me under that category. Most applications need CLI stuff and I'm not a fan of reinventing the wheel everytime I develop a new app, that's why I decided to use clap with the yaml config, as it is as much out of my way as possible.

This is what bugs me about the Derive API:

  • It requires me to create special structs for the CLI stuff
  • If my structs are logically separated in various files it makes it harder to see the full picture of what the CLI is going to end up displaying.
  • My structs are harder to read and understand becuase CLI related stuff is above every property.

I am interested in hearing more about in which ways you appreciate yaml helping separate clap / app logic and why it is important.

I like it because:

  • The whole CLI "boilerplate logic" is contained in one single yaml file.
    • All of the CLI logic and what it's going to display can be found in this single file.
  • The yaml file structure is very easy to read and understand.
  • I need just a single line to load all of the matches from the CLI arguments:
let matches = App::from(load_yaml!("cli.yaml")).get_matches();
  • Even the CLI restrictions (e. g. required arguments, default values, environment variables, etc.) can be declared inside the yaml file.
  • Once the matches are loaded I can just create my structs from it and I'm already done. All of the checks (e. g. required fields and default values) were already taken care of in the yaml configuration.

I over-simplified the last argument, a lot of the times you still have to do checks afterwards (e. g. if you have a directory argument you might need to check if the directory exists), but you have to do these checks with all of the other methods of loading clap matches as well, so overall it's still less "boilerplate logic".

Would you be willing to help maintain a clap_yaml or clap_serde crate?

Unfortunately no. I personally do not want to be a maintainer of public crates. Sorry!

I hope this was enough to answer all of your questions. Maybe it's best if you wait for other people to chime in once everyone gets wind of this change. 😄 I just got here early because I use the clap master branch on one of my projects.

@pvshvp-oss
Copy link

YAML was nice to have. I use it too. It helped me separate out the CLI boilerplate from the main code. I am disappointed to see it go :(

@tshepang
Copy link
Contributor

I think it works well as a separate component... I always thought it overkill to have it as part of clap.

@HitomiTenshi
Copy link

@tshepang Well it is behind an optional feature flag in v3 (not part of default features), so you usually wouldn't compile it unless you specifically included it.

@tshepang
Copy link
Contributor

Unfortunately no. I personally do not want to be a maintainer of public crates. Sorry!

why not

@HitomiTenshi
Copy link

why not

Because I do not want to do it. It's that simple. No excuses here. 😜

@MitaliBo
Copy link

I also liked yaml way of parsing the cli arguments and have you main logic only in .rs file. It was so convenient to read and quickly add a new subcommand and args, even the person do not have much knowledge of rust code can do it . Very disappointed to see that its support is discontinued :( Probably i would like to maintain it as separate crate if i see there are avid users of it like me :)

@DuckEater54
Copy link

Is there any decent crate released for clap with yaml yet? Also, how would you use the clap::Parser trait to parse and run a yaml file for your clap app

@epage
Copy link
Member Author

epage commented Jan 2, 2022

Is there any decent crate released for clap with yaml yet?

The most I've seen is @MitaliBo posting right before you expressing interest in starting such a crate.

btw something that might be of interest is rustdoc's unstable json output could possibly be used to help seed some code-gen for a yaml parser which could help in keeping yaml support up-to-date. The main problem with it at the moment is it panics on clap, see crate-ci/cargo-crate-api#3.

Also, how would you use the clap::Parser trait to parse and run a yaml file for your clap app

At the moment, there is no integration between clap::Parser and YAML support. Clap 3's yaml support is nearly exactly what it was for clap 2.

@abakay
Copy link

abakay commented Jan 3, 2022

Besides all previous very important points: reduce boilerplate code, easy to read/maintain all command-line options meta info in one clear YAML file, etc. I want to highlight other point. Because it is easy to parse YAML file and get some meta data (like command-line option names, etc.), it was very easy/ergonomic to merge clap parsing results with other parsers from other sources (file, environment variables, etc.). Please see the following crate for examples: irx-config.

Without programmatic API to get meta info from clap parser, it will greatly increase the amount of boilerplate code and could/will create potential discrepancies if users will have to specify some meta info twice manually vs program will read such meta info from same source (aka YAML file).

@epage
Copy link
Member Author

epage commented Jan 3, 2022

Without programmatic API to get meta info from clap parser, it will greatly increase the amount of boilerplate code and could/will create potential discrepancies if users will have to specify some meta info twice manually vs program will read such meta info from same source (aka YAML file).

You can also read metadata from a clap::App which also works with derives because deriving Parser also derives IntoApp which allows you to call IntoApp::into_app to get a clap::App.

@epage
Copy link
Member Author

epage commented Jan 5, 2022

Just noticed that tauri has its own serde api on top of clap. While not the most efficient design (serializes into structs . and then uses the builder), it might be a good starting point for generalizing the API.

Seems to be using fairly standard licenses.

@Licenser
Copy link

I understand the reasoning, but doing an impactful change like deprecating an entire feature with a one day comment period right at the end of a beta before release is quite painful for users :( for future versions could large changes like that be at least announced early on in the beta/RC candidate phase it'd make it easier to deal with for users.

@tshepang
Copy link
Contributor

I would be more sympathetic if this was a removal, but it is a mere deprecation, something that also acts as an honest indicator of where things stand... doing otherwise would be misleading. I also think maintainers should be allowed to break beta releases at any time, and I don't think the deprecation was done 'late' deliberately.

@epage
Copy link
Member Author

epage commented Jan 11, 2022

A couple of points

  • There were some specific unique circumstances around development in this case
  • Lines of communication in open source are difficult. A recent example of this is the Python cryptography package moving to Rust, "breaking" people. Deprecation messages are our clearest, most complete way of communicating this
    • My hope is we can more smoothly go through major releases such that we will have minimal to no beta / RC releases in the future. Most development should focus on deprecations / feature flags.
  • We did give 23 days between issue creation and release, the more relevant time window since the release is what locks us in for a period of time. That 23 days was not even prescribed; we would have held it up if 3.0 was not deemed ready yet due to any outstanding breakages or major regressions.
  • This is also a fairly special change in that there isn't much room for feedback. This is a wholly distinct API with its own design, documentation, and stability needs and considerations. This doesn't belong in clap. The only alternative on the table is we jump start the split out into a new crate.

@Licenser
Copy link

Ja, I got that wrong two things overlapped from was turned into from_yaml so that and the deprication warning message made me think this was removed not just depricated :) sorry about that - it was just marked as depricated so my frustration was misplaced .

philn added a commit to philn/opentok-rs that referenced this issue Jan 17, 2022
The Clap YAML API was deprecated, see:
clap-rs/clap#3087

We should move to the Parser API instead, even though YAML was nice... to be
done later on.

Also pin to latest GStreamer stable releases. Using git snapshots shouldn't be
needed, unless we need un-released fixes.
philn added a commit to philn/opentok-rs that referenced this issue Jan 18, 2022
The Clap YAML API was deprecated, see:
clap-rs/clap#3087

We should move to the Parser API instead, even though YAML was nice... to be
done later on.

Also pin to latest GStreamer stable releases. Using git snapshots shouldn't be
needed, unless we need un-released fixes.
@aobatact
Copy link

I made a crate for deserializing clap. https://crates.io/crates/clap-serde

@epage
Copy link
Member Author

epage commented Jan 21, 2022

Sounds great!

I tried to look into the details but it looks like the repo link is broken.

@aobatact
Copy link

Oh I forgot it to make it public...
https://github.com/aobatact/clap-serde

@epage
Copy link
Member Author

epage commented Jan 21, 2022

@aobatact I've done a quick pass over it and noted some things. I've also

Once you feel its in a good enough state, we can raise the visibility further, like linking to it from the README/docs.rs

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

No branches or pull requests

10 participants