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

Upgrade bpaf to 0.7 to remove some turbofishes #92

Merged
merged 4 commits into from
Nov 3, 2022

Conversation

pacak
Copy link
Contributor

@pacak pacak commented Oct 11, 2022

Plus various code improvements, feel free to drop some or all of them.

pacak added 2 commits October 11, 2022 11:45
Some result in a slightly better generated code, some make it less likely to
explode. Or more readable.
@pacak
Copy link
Contributor Author

pacak commented Oct 11, 2022

Do you want to update the code to use derive macro? This means compiling syn, but it's already being compiled...

@Shnatsel
Copy link
Member

Thanks! I'll take a look shortly.

Re: derive macro: I'm less concerned about compiling syn as I am about the macro being re-run on every compilation, increasing the incremental compilation times. I'm not very familiar with how derive macros work, so if that's not an issue, then using a derive macro to reduce the amount of code would be great.

@pacak
Copy link
Contributor Author

pacak commented Oct 11, 2022

https://github.com/rosetta-rs/argparse-rosetta-rs according to this other than initial cost to compile syn and related stuff - there's no difference to incremental compile time.

@Shnatsel
Copy link
Member

In that case using the derive API would be great! We already use syn for serde, so we just get to maintain less boilerplate for free.

@pacak
Copy link
Contributor Author

pacak commented Oct 11, 2022

In that case using the derive API would be great!

Okay. Gimme a day or two.

@Shnatsel
Copy link
Member

Thanks a lot!

@pacak
Copy link
Contributor Author

pacak commented Oct 12, 2022

Pushed, wording might change in some places but cli tests are passing as is so hopefully nothing breaks.

Also added a way for users to make cli parser output more colorful a-la clap3/clap4 if they want to. No extra dependencies by default.

@Shnatsel
Copy link
Member

It's a little strange to resolve color as a compilation feature. This is usually runtime configuration - and is disabled by default when writing to something other than a terminal, so that the plain text file doesn't end up littered with terminal escape sequences. See e.g. the behavior of grep --color= where it defaults to auto, but can be explicitly enabled or disabled.

Is there a way to do runtime resolution with bpaf? If not, I'd rather stick to plain/colorless output so that the output dumped to a text file would still be readable.

@pacak
Copy link
Contributor Author

pacak commented Oct 12, 2022

Runtime detection is present as well and will be used if you enable either colorscheme so redirect to file or running in CI would work as expected. Having it as a compile time helps people to decide if they want any colors at all - I would prefer no colors so won't be enabling this feature, but some people like it.

@Shnatsel
Copy link
Member

My apologies for the delay, I got caught up in RL stuff. I'll try to review and merge this next week.

@pacak
Copy link
Contributor Author

pacak commented Oct 14, 2022

My apologies for the delay, I got caught up in RL stuff. I'll try to review and merge this next week.

No worries, take your time.

@pacak
Copy link
Contributor Author

pacak commented Nov 2, 2022

Any thoughts? Also do you want a pull request for cargo-auditable? pico-args is simple but correctness is not quite there - see RazrFalcon/pico-args#15 for example.

@Shnatsel
Copy link
Member

Shnatsel commented Nov 2, 2022

Thank you for the reminder, I'll try to take a look in the next few days now that I've shipped cargo audit 0.17.3.

cargo auditable doesn't need any help generation, and it only captures a subset of all arguments passed. Basically I'm intercepting only some of the arguments passed to rustc, for which pico-args is a great fit.

I am aware of the correctness issue you mentioned, but it only matters for invalid argument sequences, and rustc itself already does an argument validation pass so we don't have to.

@Shnatsel
Copy link
Member

Shnatsel commented Nov 3, 2022

Looks good to me! I'm going to merge this and cut a release. Thanks a lot!

I'm going to make the stylistic choice of bright colors, and drop the feature. I think that's the option that's easiest to skim, and the colors chosen are readable on both light and dark backgrounds. Handling of piping also looks correct to me.

@Shnatsel Shnatsel merged commit 4830980 into rust-secure-code:master Nov 3, 2022
@pacak
Copy link
Contributor Author

pacak commented Nov 3, 2022

I'm going to make the stylistic choice of bright colors, and drop the feature.

There's no downside to leaving the feature in even if you go with the bright colors by default, for example I would prefer not having color at all compared to the bright option. Visibility depends on only on background being dark or bright, but also on a palette.

@pacak
Copy link
Contributor Author

pacak commented Nov 3, 2022

@pacak
Copy link
Contributor Author

pacak commented Nov 3, 2022

Also if you really want to drop the color selection - you need to update the docs.

@Shnatsel
Copy link
Member

Shnatsel commented Nov 4, 2022

Actually, I'll have to disable color for now because it pulls in atty, which has known UB and appears to be unmaintained: softprops/atty#50

@pacak
Copy link
Contributor Author

pacak commented Nov 4, 2022 via email

@Shnatsel
Copy link
Member

Shnatsel commented Nov 4, 2022

No, but I haven't really looked.

You could always just fork it and we can encourage people to switch via the RustSec advisory for example: rustsec/advisory-db#1457

@Shnatsel
Copy link
Member

Shnatsel commented Nov 4, 2022

I've shipped v0.3.2 - without color, but it does update to bpaf 0.7.x on crates.io.

I'd be happy to add color later because I think it meaningfully improves the readability of the help text.

@pacak
Copy link
Contributor Author

pacak commented Nov 4, 2022

I'll see what I can do about atty - I have some other changes planned first. Maybe a week or two.

@pacak
Copy link
Contributor Author

pacak commented Nov 23, 2022

I did this for now FWIW zkat/supports-color#9

@pacak
Copy link
Contributor Author

pacak commented Dec 15, 2022

supports-color pull request got merged, hopefully a release soon.

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.

2 participants