TeXitoi / structopt

Parse command line arguments by defining a struct.
Other
2.7k stars 149 forks source link

Custom Structopt Crate Path #506

Closed kszela24 closed 2 years ago

kszela24 commented 2 years ago

This PR provides the initial work around passing structopt_path to the procedural macro (https://github.com/TeXitoi/structopt/issues/339).

There's two places left that I'm having trouble seeing where I can pull the structopt_path attribute in:

https://github.com/TeXitoi/structopt/blob/da1fff81aded1c239ffcbd0a27ccdc7f28f74ff2/structopt-derive/src/lib.rs#L993-L1004

Not entirely sure how to handle this, I'm guessing the dummy returned by this block wouldn't be ideal for a user if it ended up getting used in a package built on top of structopt?

The other location is in attrs.rs:

https://github.com/TeXitoi/structopt/blob/da1fff81aded1c239ffcbd0a27ccdc7f28f74ff2/structopt-derive/src/attrs.rs#L322-L331

Additionally, I had tried using crate as serde does in their implementation but kept running into a compilation error: "expected identifier" (which I guess creates an issue because it's a keyword) so went with this naming. Not sure what's best to name it if not crate, so went with structopt_path. It might be due to the way that serde is parsing the macro attributes but the way I've done it is consistent with the rest of the cases for parsing the arguments. Happy to give it another go if you'd like this to be consistent with serde.

kszela24 commented 2 years ago

Also happy to add a small section in the documentation around extending structopt and using this new functionality

TeXitoi commented 2 years ago

Looks good. Need documentation.

The dummy implementation is to limit cascading errors. It will not work, bit I don't see how to workaround this, and that's not critical, it will just add errors. Not great bit we can live with it.

For the second, if the path is given after, too bad. Just specify in the documentation that the path must be specified first. I suppose this is the problem you have, and I suppose you can call the structopt_path method here easily, else I wrongly understand the problem about this one.

Thanks a lot for your contribution!

TeXitoi commented 2 years ago

I also supposebyou tested it? Shouldn't a test can be added with a export of structopt in a module, just to check that's working (even if the test might not catch errors so well as a forgotten path will obviously work).

For the crate name, maybe that's an edition thing to have crate as a keyword. structopt_path looks good to me.

kszela24 commented 2 years ago

Got it, makes sense regarding the dummy implementation.

Sounds good about explicitly mentioning it in the docs. I’ll try implementing what you’re suggesting today.

Yep I’d been using a separate test crate for testing and will add unit tests and documentation.

Thanks for the review and help/suggestions! Hopefully just one more review once I’ve completed the above.

TeXitoi commented 2 years ago

This is an enhancement, and structopt is now feature frozen. Please propose it to clap.

Thanks for your contribution!