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

replace history --command with history command #3367

Closed
krader1961 opened this issue Sep 11, 2016 · 17 comments
Closed

replace history --command with history command #3367

krader1961 opened this issue Sep 11, 2016 · 17 comments
Assignees
Milestone

Comments

@krader1961
Copy link
Contributor

The use of long options for history subcommands is an anti-pattern (i.e., so unusual it serves as an example of what not to do). It appears the original justification for doing so was to allow invoking more than one subcommand with a single history command. However, no one liked that as it was ambiguous, confusing, and made it too easy to shoot yourself in the foot. So a recent change was merged that only allows one of the long options that specifies a subcommand.

This issue is to take the next logical step and actually introduce subcommands; e.g., history search rather than history --search. The long option form will still be supported for the foreseeable future but will be deprecated. As part of this change we'll also remove the short options for the subcommands per this comment. That includes removing, for consistency, history -s to mean history --search or history search.

@krader1961 krader1961 added this to the fish 2.4.0 milestone Sep 11, 2016
@krader1961 krader1961 self-assigned this Sep 11, 2016
@krader1961
Copy link
Contributor Author

Note that there is one backward incompatible aspect to this proposal: history search something will no longer search for search something, it will search for something. Similarly history delete something will no longer search for delete something it will instead delete an entry that exactly matches something (assuming neither --contains or --prefixes is specified).

@c22
Copy link

c22 commented Sep 12, 2016

I agree with the sentiment of this issue, though it is unfortunate that the change would be backward incompatible. I will add that switching to a subcommand pattern isn't mutually exclusive to having (or not having) shorthand syntax.

eg. history search == history s

Tab completion is a good argument against the need for shorthand syntax though.

@krader1961
Copy link
Contributor Author

Yes, but how often do you search for the word "search" or "delete"? Also, the long option form (e.g., --search) will remain valid for the foreseeable future; it will simply be deprecated and no longer prominent in the documentation. Also, the "search" subcommand keyword will be optional unless you want to search for the word "search".

@krader1961
Copy link
Contributor Author

Also, note that supporting short versions of the keywords increases the number of words that cannot be implicitly searched for without having to say history search. Tab completion or an abbreviation (e.g., abbr --add hs 'history search') seem like better ways to minimize the amount the user has to type.

@c22
Copy link

c22 commented Sep 12, 2016

Agreed. Also I'm just one user of fish, but I almost exclusively use archaic form history | grep and when I use builtin search functionality it's usually with the modifiers such as history --prefix. To that end, intuitively I would expect history --prefix search to be expanded to history search --prefix search though the amount of times I can ever see myself doing that in practice is close to none.

@krader1961
Copy link
Contributor Author

Note that due to how flags are parsed by the history function and builtin doing something like history --prefix search will actually search for the word "search"; i.e., it's equivalent to history search --prefix search. So that behavior won't be changed. Note that --prefix does not take any arguments so history --prefix --with-time search also works. This is something that was ambiguous in the old history man page which I hope I've clarified with my recent change.

P.S., It's also a good example of why I feel very strongly that other places where fish allows flags to appear anyplace on the command line should be changed to stop processing flags after the first non-flag argument.

@c22
Copy link

c22 commented Sep 12, 2016

stop processing flags after the first non-flag argument

I believe the POSIXLY_CORRECT behaviour of GNU getopts behaves in a similar way. Making fish more POSIXy aligns nicely with high level goals of fish as a well.

@floam
Copy link
Member

floam commented Sep 13, 2016

Overly strict argument requirements which reject the user when we could otherwise figure it out don't strike me as a particularly worthy goal. It's something we might be stuck with if we can't "figure it out" - but for instance not being able to handle history search foo --prefix or history search foo --with-time strikes me as unhelpfully pedantic. Ideally we should not have to worry much about order. In my own history I see a lot of options at the end of the commands - typically I hit the up arrow and then an argument to adjust the results.

It'd be nice if we are changing up the syntax here to see what we can do to loosen it up.

@krader1961
Copy link
Contributor Author

Overly strict argument requirements which reject the user when we could otherwise figure it out...

I strongly dislike the wgetopt_long() default behavior to permute the sequence of tokens and thus allow options to appear anywhere on the command line. For example, if I modify builtin_history() by removing the leading + in the short option string the following causes the -t to be treated as the -t (i.e., --with-time) flag rather than a string to be searched for:

builtin history search wtf -t

You, @floam, on the other hand like that behavior. Given that almost all of our wgetopt_long() long invocations implement the behavior you prefer I'll change builtin_history() to match. But I'm wondering if the existing behavior was a conscious decision or an accident.

Too, note that the POSIX mandated behavior is my preferred behavior. It's also what is easiest to implement in a fish script such as share/functions/history.fish. I'll open another issue to discuss this and whether we should modify fish C++ code to uniformly use a leading +, -, or neither. Please don't argue for one of those behaviors here as it is orthogonal to the point of this issue. We can have that discussion in the issue I intend to open (assuming I don't find an already open issue on this topic). I'm simply going to change the existing builtin_history() behavior to match that of most of the fish builtins as part of addressing this issue.

@floam
Copy link
Member

floam commented Sep 17, 2016

One thing that's wrong in string, is any command that takes subcommands ought to take command help. string help is not valid -- but our help system prints out the usage section of the manpage in the case of a argument error so it often "works", but explicitly calling --help is supposed to return the entire manpage.

So this will really not work if the user hoped to get details about match: string help match

This was kind of a surprise given the length of the manpage.

We'll want to get that right for this.

As part of this change we'll also remove the short options for the subcommands per this comment.

I didn't suggest removing the good ones.

history search -t (so even history search foo -t) should exist. -t is easy to type, short options aren't problematic for command-line search utilities -- they're key: I doubt many of us are confused by or prefer not to use our go-to grep options when we are searching other logs and I doubt they are --long.

(And if grep foo bar -n breaks POSIX and can only be grep -n foo bar by the book -- I guess that wouldn't be a surprise. The ambiguity isn't problematic, to me at least it is obvious that "of course" when my intended search includes a literal option in it I think to use '--' and/or quote the phrase.

@floam
Copy link
Member

floam commented Sep 17, 2016

@krader1961 Wait, do you mean short options aliased to subcommands? My reply above assumed you meant that they'd not work when following a subcommand/always - I didn't see -t used anywhere but did see --with-time which didn't help.

@krader1961
Copy link
Contributor Author

krader1961 commented Sep 18, 2016

@floam, I meant that this change will remove the short option equivalent for the flags that represent subcommands. For example, history --search will still be valid but history -s will not be valid. The options that apply to a specific command (e.g., -p) will continue to have both short and long forms.

Regarding your example involving grep there are good arguments for both behaviors with respect to recognizing, or not, the -n flag in arbitrary locations. I personally prefer the POSIX behavior. But I can live with either behavior. I'm simply pointing out that at the moment fish is slightly inconsistent and the documentation is totally silent on the matter. Please don't reply here as the topic is orthogonal to this issue. As soon as I find a relevant open issue or open a new one we can continue this discussion there.

@olbrew
Copy link

olbrew commented Nov 1, 2016

@krader1961

Yes, but how often do you search for the word "search" or "delete"?

I'm on 2.4b1 and actually wanted to delete all uses of the "--delete" from my history. (Why should it even be added to the history in the first place? If you want to delete a certain operation from your history it implies that you probably don't want the delete operation, mentioning the terms you wanted to delete in the first place, to show up in your history.)
But it seems impossible with the search term "--delete", quoted or not, both with the new subcommand and old option syntax.

@faho
Copy link
Member

faho commented Nov 1, 2016

@olbrew: Tried history delete -- --delete? From a quick reading of the function, that should work.

@krader1961
Copy link
Contributor Author

@olbrew: @faho's suggestion will work. All fish commands (but not necessarily all commands on your system) support -- to terminate parsing the command line for flags. Simply quoting the argument doesn't help because the command parser strips the quotes as it splits the command line into tokens before passing the tokens to the command. So the history command still gets --delete without quotes.

As for why it's added to the history in the first place it's a question of consistency and what is least surprising to a user. As soon as you start special-casing things like history --delete the documentation gets longer and it's more likely someone will be surprised. The KISS principle usually trumps most other considerations.

@olbrew
Copy link

olbrew commented Nov 1, 2016

@faho Thank you, that worked indeed.

@krader1961

Simply quoting the argument doesn't help because the command parser strips the quotes as it splits the command line into tokens before passing the tokens to the command. So the history command still gets --delete without quotes.

That makes sense I'll remember it for the future!

As for why it's added to the history in the first place it's a question of consistency and what is least surprising to a user. As soon as you start special-casing things like history --delete the documentation gets longer and it's more likely someone will be surprised. The KISS principle usually trumps most other considerations.

Didn't think about it like that but I definitely have to agree now. I'll just keep manually deleting them then :).

@c22
Copy link

c22 commented Nov 1, 2016

@olbrew Handy tip, if you don't want a command to show in your history (at least under default conditions), just add a space to the start of it.

So when you do a history --delete just add a space before the history command to omit it from the history.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants