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

Clarify help message for --time option #125

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

Expurple
Copy link
Contributor

@Expurple Expurple commented Mar 25, 2024

Inspired by a confused user on Reddit.

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

@ahadley1124
Copy link

I do prefer this new wording

@admalledd
Copy link

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

  • rdiff-backup --remove-older-than (more verbose option name as well, but expected since this tool is meant to be scripted the once, rarely ran interactively if at all)
    • Remove the incremental backup information in the destination directory that has been around longer than the given time. [SNIP]

    • IMO, I would partly wonder about changing the --time argument's name itself, but that is very much out of scope of a quick PR like this. Something more akin to --keep time_spec but that runs into not uncommon "I want to keep by file pattern" confusion. I am less than helpful at naming things :) so maybe keep --time after all.
  • git-clone --shallow-since
    • Create a shallow clone with a history after the specified time.

    • maybe not a great sample, but was in my HISTORY file
  • journalctl --since|--until
    • Start showing entries on or newer than the specified date, or on or older than the specified date, respectively. [SNIP]

    • Again where what is written is the least surprising action (which is helped by the arg name itself)
  • docker image prune --filter
    • [SNIP] until (<timestamp>) only remove images created before given timestamp [SNIP]

    • Partly more an anti-example, since the expectation is that filters are more-or-less generated by other applications/front-ends to docker CLI.
  • go clean does not provide any preserve options it seems (which makes sense for a package manager)
  • npm prune also does not provide any preserve options (which makes sense for a package manager)
  • borg prune --keep-within
    • keep all archives within this time interval

    • A close intent and wording, 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.
  • outlook email
    • has some sample images, verbiage below
    • Clean out items older than ...

    • Find items older than ...

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
Copy link
Contributor Author

Expurple commented Mar 26, 2024

@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
Copy link

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
Copy link
Collaborator

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.

@marcospb19 marcospb19 merged commit e1761a4 into holmgr:master Mar 27, 2024
9 checks passed
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.

4 participants