make `status` saner vis-a-vis arg parsing #3526

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@krader1961
Contributor

krader1961 commented Nov 6, 2016

Note

Now that we have two sets of functions that perform string-to-enum and enum-to-string translations it makes sense to generalize this and make functions that take a table which defines the legal mappings. I'll do that in a separate change.

Description

The status command currently silently allows incompatible flags (i.e.,
subcommands). Too, using flags to specify subcommands misleads the user
into thinking they can specify multiple subcommands.

We recently modified the history command to deprecate using flags for
subcommands. This change does the same for the status command.

Fixes #3509

make `status` saner vis-a-vis arg parsing
The `status` command currently silently allows incompatible flags (i.e.,
subcommands). Too, using flags to specify subcommands misleads the user
into thinking they can specify multiple subcommands.

We recently modified the `history` command to deprecate using flags for
subcommands. This change does the same for the `status` command.

Fixes #3509
@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Nov 6, 2016

Member

If we're going to change it, I think that there should be a more rational set of shared subcommands. Like status is <property>.

Member

floam commented Nov 6, 2016

If we're going to change it, I think that there should be a more rational set of shared subcommands. Like status is <property>.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Nov 6, 2016

Contributor

I think that there should be a more rational set of shared subcommands.

I disagree since that will merely make parsing the command more difficult should we decide to allow flags that are specific to a subset of subcommands. Not to mention greatly complicating the code to maintain backward compatibility with the existing flags. We're not inventing a DSL (domain specific language) here where that sort of grammar could be justified. You're overthinking this.

Contributor

krader1961 commented Nov 6, 2016

I think that there should be a more rational set of shared subcommands.

I disagree since that will merely make parsing the command more difficult should we decide to allow flags that are specific to a subset of subcommands. Not to mention greatly complicating the code to maintain backward compatibility with the existing flags. We're not inventing a DSL (domain specific language) here where that sort of grammar could be justified. You're overthinking this.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Nov 6, 2016

Contributor

Also, there is absolutely nothing to be gained in this situation from using status is login versus status is-login or status is_login.

Contributor

krader1961 commented Nov 6, 2016

Also, there is absolutely nothing to be gained in this situation from using status is login versus status is-login or status is_login.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Nov 6, 2016

Member

Otherwise, I don't really see much advantage to changing this. It seems unnecessary.

Member

floam commented Nov 6, 2016

Otherwise, I don't really see much advantage to changing this. It seems unnecessary.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Nov 6, 2016

Contributor

Otherwise, I don't really see much advantage to changing this. It seems unnecessary.

See my comment in the issue about how using flags misleads people into thinking they can specify more than one to select multiple subcommands. It's also inconsistent with how most other commands (e.g., git) handle subcommands.

Contributor

krader1961 commented Nov 6, 2016

Otherwise, I don't really see much advantage to changing this. It seems unnecessary.

See my comment in the issue about how using flags misleads people into thinking they can specify more than one to select multiple subcommands. It's also inconsistent with how most other commands (e.g., git) handle subcommands.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Nov 6, 2016

Contributor

This change does not force anyone to switch from the flag to non-flag form for selecting a subcommand. At least not until we bump the major version and perhaps not even then. It simply makes it much clearer that you can only specify a single subcommand per invocation.

Contributor

krader1961 commented Nov 6, 2016

This change does not force anyone to switch from the flag to non-flag form for selecting a subcommand. At least not until we bump the major version and perhaps not even then. It simply makes it much clearer that you can only specify a single subcommand per invocation.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Nov 6, 2016

Contributor

Also, in case it isn't already clear, this change (and the equivalent change to the history command made a few weeks ago) will require at least two years to pass before we can remove the flag form for specifying a subcommand even if we bump the major number before then. This merely starts the clock running so that at some point in the future we can drop the backward compatible support.

Contributor

krader1961 commented Nov 6, 2016

Also, in case it isn't already clear, this change (and the equivalent change to the history command made a few weeks ago) will require at least two years to pass before we can remove the flag form for specifying a subcommand even if we bump the major number before then. This merely starts the clock running so that at some point in the future we can drop the backward compatible support.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Nov 6, 2016

Member

If we're going to change it, I think that there should be a more rational set of shared subcommands. Like status is .

@floam: For what the status command offers, that'd just be annoying. We currently have 7 "is" tests, 3 things that print some introspection stuff and one thing that sets an option.

So you'd do status is interactive, status is login, status is full-job-control and status print filename/status print line-number. That just seems a bit overkill (especially wrt completions, since you would need to first complete "is" to then see what else is available).

Personally, I'm not a huge fan of either of these, but if it works I'll accept it since @krader1961 wants it.

@krader1961: One thing this is lacking is an update to the completions. We can do that later, though.

Member

faho commented Nov 6, 2016

If we're going to change it, I think that there should be a more rational set of shared subcommands. Like status is .

@floam: For what the status command offers, that'd just be annoying. We currently have 7 "is" tests, 3 things that print some introspection stuff and one thing that sets an option.

So you'd do status is interactive, status is login, status is full-job-control and status print filename/status print line-number. That just seems a bit overkill (especially wrt completions, since you would need to first complete "is" to then see what else is available).

Personally, I'm not a huge fan of either of these, but if it works I'll accept it since @krader1961 wants it.

@krader1961: One thing this is lacking is an update to the completions. We can do that later, though.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Nov 6, 2016

Member

Also, one piece of weird behavior:

> status is-interactive --current-filename
status: current-filename expected 0 args, got 1

i.e. the error is about the last operation, where I'd expect it to be about the first.

Member

faho commented Nov 6, 2016

Also, one piece of weird behavior:

> status is-interactive --current-filename
status: current-filename expected 0 args, got 1

i.e. the error is about the last operation, where I'd expect it to be about the first.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Nov 6, 2016

Contributor

the error is about the last operation, where I'd expect it to be about the first.

Actually, that particular combination should complain about specifying two subcommands in the same invocation. Pretty sure I know what I did wrong. I made the same mistake with the history builtin but not the function. Since I basically copy-pasted the history code it's no surprise you're seeing this behavior. Fix forthcoming for both builtin commands.

I actually started working on revamping the completions but wanted to get the core of the change out for feedback. Completions will be updated later today.

We certainly don't have to introduce non-flag subcommands in order to fix the core problem. But imagine if we had made the string command use flags for the subcommands. Instead of string match you'd be writing string --match. I think everyone would agree that's bizarre. The only reason you don't feel that way about status --is-login is that it's been that way for several years. With this change some additional changes I'm contemplating to extend the behavior of current-filename and current-line-number result in a more natural syntax.

Contributor

krader1961 commented Nov 6, 2016

the error is about the last operation, where I'd expect it to be about the first.

Actually, that particular combination should complain about specifying two subcommands in the same invocation. Pretty sure I know what I did wrong. I made the same mistake with the history builtin but not the function. Since I basically copy-pasted the history code it's no surprise you're seeing this behavior. Fix forthcoming for both builtin commands.

I actually started working on revamping the completions but wanted to get the core of the change out for feedback. Completions will be updated later today.

We certainly don't have to introduce non-flag subcommands in order to fix the core problem. But imagine if we had made the string command use flags for the subcommands. Instead of string match you'd be writing string --match. I think everyone would agree that's bizarre. The only reason you don't feel that way about status --is-login is that it's been that way for several years. With this change some additional changes I'm contemplating to extend the behavior of current-filename and current-line-number result in a more natural syntax.

@krader1961 krader1961 added the bug label Nov 6, 2016

@krader1961 krader1961 added this to the next-2.x milestone Nov 6, 2016

add completions and fix arg parsing
This updates the completions for the `status` command. It also corrects
the handling by the builtin `history` and `status` commands of a
subcommand specified via both a flag and a non-flag argument.
@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Nov 7, 2016

Contributor

@faho: Please review the updated the completions. The one thing I couldn't figure out is how to inhibit file name completion after a subcommand is seen which doesn't require, or accept, any further arguments.

Contributor

krader1961 commented Nov 7, 2016

@faho: Please review the updated the completions. The one thing I couldn't figure out is how to inhibit file name completion after a subcommand is seen which doesn't require, or accept, any further arguments.

+function __fish_status_needs_job_ctrl_cmd

This comment has been minimized.

@faho

faho Nov 7, 2016

Member

This is basically just __fish_seen_subcommand_from job-control.

@faho

faho Nov 7, 2016

Member

This is basically just __fish_seen_subcommand_from job-control.

+# The job-control command changes fish state.
+complete -f -c status -n "not __fish_seen_subcommand_from $__fish_status_all_commands" -a job-control -d "Set which jobs are under job control"
+complete -f -c status -n "__fish_status_needs_job_ctrl_cmd" -a full -d "Set all jobs under job control"
+complete -f -c status -n "__fish_status_needs_job_ctrl_cmd" --no-files -a interactive -d "Set only interactive jobs under job control"

This comment has been minimized.

@faho

faho Nov 7, 2016

Member

These "--no-files" are duplicates of the earlier "-f".

@faho

faho Nov 7, 2016

Member

These "--no-files" are duplicates of the earlier "-f".

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Nov 7, 2016

Member

The one thing I couldn't figure out is how to inhibit file name completion after a subcommand is seen which doesn't require, or accept, any further arguments.

Currently, you don't - see #112.

The rest seems logically okay, I just had two nits.

Member

faho commented Nov 7, 2016

The one thing I couldn't figure out is how to inhibit file name completion after a subcommand is seen which doesn't require, or accept, any further arguments.

Currently, you don't - see #112.

The rest seems logically okay, I just had two nits.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Nov 7, 2016

Contributor

Merged as 9e922a6.

Contributor

krader1961 commented Nov 7, 2016

Merged as 9e922a6.

@krader1961 krader1961 closed this Nov 7, 2016

+ case STATUS_PRINT_STACK_TRACE:
+ return L"print-stack-trace";
+ }
+}

This comment has been minimized.

@zanchey

zanchey Nov 8, 2016

Member

openSUSE builds are complaining about no return in the default case again, and failing to build.

@zanchey

zanchey Nov 8, 2016

Member

openSUSE builds are complaining about no return in the default case again, and failing to build.

This comment has been minimized.

@krader1961

krader1961 Nov 8, 2016

Contributor

I'm just about to create a PR that generalizes that code using table driven template methods. That should take care of those false positives.

@krader1961

krader1961 Nov 8, 2016

Contributor

I'm just about to create a PR that generalizes that code using table driven template methods. That should take care of those false positives.

This comment has been minimized.

@krader1961

krader1961 Nov 8, 2016

Contributor

Also, any chance we can setup an existing or new Travis build environment to use the same tool chain so we can get Travis to tell us about these problems?

@krader1961

krader1961 Nov 8, 2016

Contributor

Also, any chance we can setup an existing or new Travis build environment to use the same tool chain so we can get Travis to tell us about these problems?

This comment has been minimized.

@krader1961

krader1961 Nov 8, 2016

Contributor

Well, "problems" => "non-problems" except in so far as they break the OBS build 😄

@krader1961

krader1961 Nov 8, 2016

Contributor

Well, "problems" => "non-problems" except in so far as they break the OBS build 😄

@krader1961 krader1961 deleted the krader1961:status-subcommands branch Nov 8, 2016

@faho faho added the release notes label Jan 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment