Add status current-commandline
#9296
Conversation
src/parser.h
Outdated
|
|
||
| /// Set a parser status variable | ||
| void set_status_var(parser_status_var_t var, wcstring val) { | ||
| ASSERT_IS_MAIN_THREAD(); |
There was a problem hiding this comment.
@ridiculousfish do you want me to remove this assert?
|
For context: I am not against removing $_ - it's a terrible name. However, it is still used - for example a bunch of oh-my-fish title functions do (for the record oh-my-fish still claims compatibility with fish 2.2.0, which I don't believe is helpful, but it is what it is). And given that we might want to have 3.6.0 go out sooner rather than later, because we have already found an issue with the imminent new macOS version (53cb3a9), I would not want to have this in the current cycle.
|
|
I also don’t like the current IMHO we should ship this and fix current-command together, having it return the actual command? Eg Most status current-command usages are in bindings or completions where the behavior wouldn’t change except in good ways I think, but typed out at the interactive prompt its behavior would change. There’s probably no one doing that outside of contrived demonstrations. |
I'm asking for tests for your new command, to be clear. I realize that sometimes we do things that are awkward to test, and I don't mean to require 100% tests for everything, but in this case it seems possible to do a pexpect test, and I would appreciate that. It would also help explain the differences in behavior between
I don't really have an opinion on current-command (i.e. the old one) at the moment. I find it a bit awkward to change things together like this - it introduces a new command and changes how internals work and removes a deprecated variable. But if that's what's necessary, then sure. But if the removal of |
|
$_ can be added to this easily, I’ll do it later today if I get the chance. |
mqudsi
force-pushed
the
parser_status_vars
branch
from
cbdc1fe to
2bef0d6
Compare
|
Re-added |
| @@ -239,6 +239,12 @@ struct eval_res_t { | |||
| : status(status), break_expand(break_expand), was_empty(was_empty), no_status(no_status) {} | |||
| }; | |||
|
|
|||
| enum class parser_status_var_t : uint8_t { | |||
There was a problem hiding this comment.
Unless we expect to have a lot more of these variables, I think we can do without this enum. In every call site we know which status variable we're accessing so we don't benefit from having these be indexable.
Also (as your changes illustrate) any time we want to set current_command we probably also want to set current_commandline. We can structure these so we're less likely to forget.
I suggest just making a struct:
struct status_vars_t {
wcstring command;
wcstring commandline;
};
and then you can set and get that struct. We don't even really need a setter or getter, just put it in library_data_t.
other than that this looks good to me
There should be no functional changes in this commit. The global variable `$_` set in the parser variables by `reader.cpp` and read by the `status` builtin was deprecated in fish 2.0 but kept around internally because there's no good way to store/share/forward parser variables. A new enum is added that identifies the status variable and they are stored in a private array in the parser. There is no need for synchronization because they are only set during job init and never thereafter. This is currently asserted via ASSERT_IS_MAIN_THREAD() but that assert can be dropped in the interest of making the parser possible to clone and use from worker threads. The old `$_` global variable is still kept for backwards compatibility, though it will be dropped in a future release.
Makes it possible to retrieve the currently executing command line as opposed to the currently executing command (`status current-command`). Closes fish-shell#8905.
Instead of using an enum + array, just use a struct and drop the getter and setter methods from `parser_t`.
mqudsi
force-pushed
the
parser_status_vars
branch
from
27c8bfc to
7133285
Compare
|
Totally random thing I coincidentally happened to noticed when playing around with the 3.6.0 changes: |
Removes deprecatedand stores status variables directly in the parser.$_status current-commandlinefor History is not updated when running multiple commands in the same line #8905.