holmgr / cargo-sweep

A cargo subcommand for cleaning up unused build files generated by Cargo
MIT License
690 stars 31 forks source link

Clarify help message for --time option #125

Closed Expurple closed 2 months ago

Expurple commented 2 months ago

Inspired by a confused user on Reddit.

This change is subjective, feel free do edit/reject it.

ahadley1124 commented 2 months ago

I do prefer this new wording

admalledd commented 2 months ago

Dropping in that I also misread this option previously, and scrounging around a few other "expire/preserve/past timespec" options for wording examples:

All to say, the wording varies greatly from application to application, and I couldn't off-hand find decent samples that were closer to intent in design/usage as what cargo-sweep does, but those by backup software seem a decent (if slightly inverse) fit to confirm the new wording is more aligned with expectations and first impression understanding.

Expurple commented 2 months ago

@admalledd, quality comment. Good points, including:

of course a backup software prefers to be more about what it is keeping vs deleting, so that here for cargo-sweep that the wording is more or less inverse makes sense to me.

Some of the examples are not really applicable, because they specify an absolute date rather than a time period (which makes more sense for cargo-sweep's use case). Though, I really liked the first example with rdiff-backup --remove-older-than. Yeah, an ideal interface would look something like cargo sweep --remove-older-than 30d.

However, I definitely wouldn't rename --time to avoid breaking shit. I personally have a cron job with cargo sweep, I wouldn't want all similar scripts to break. At most, we could add an alias and support for optional unit suffixes, in a backward-compatible way. But I'd say that my PR already achieves 90% of the benefit. No one remembers these option names anyway. A good enough --help is a good enough solution, IMO.

admalledd commented 2 months ago

I agree on "don't rename --time", I actually don't like the overly verbose command line args unless the tool is nearly always expected to be setup in cron/script/systemd.timer/etc use case. Such as rarely if ever actually meant to be directly invoked by human. The shorter arg is a good thing here in my opinion (that it also short-hands to -t is of course nice). I can't think of any reasonable way to phrase as shortly as the existing --time the remove-older-than type wording of the arg itself. Any other options (like --keep) I have problems with potential mixup with "keep what? path spec/patterns? time?" that would then require extending the name anyways to imply the time component that any change becomes moot. Of course, that is all I can think of, as always one of the hardest programming things is naming stuff :) but certainly for now, just updating the description is plenty. if-when (probably never, not worth it) someone very clever with words comes up with better naming that can be a future problem, in a future issue/PR, not this one.

I agree most of the samples are less applicable, when I started writing I thought I would find more in my HISTORY or quick searches, so I went a bit wider/further afield and even listed a few that while intuition might have had a keep/preserve/timespec turns out didn't (most notably other full package managers, npm, nuget, go). At that point spent enough moments that thought worth completing anyways, even if most didn't fit exactly, to see if anyone else had inspiration.

marcospb19 commented 2 months ago

Thanks for the input folks, it's possible to change it to --remove-older-than 30 or --remove-older 30 without breaking backward compatibility, like @Expurple mentioned, by creating an alias to the previous flag name, however...

No one remembers these option names anyway. A good enough --help is a good enough solution, IMO.

I agree with this, and therefore I'm leaning towards keeping --time as it is.

Let me know if y'all have any strong opinions bout this and we can reconsider :) .

And thanks for the PR too.