-
Notifications
You must be signed in to change notification settings - Fork 715
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
Fix inconsistent argument parsing #5189
base: master
Are you sure you want to change the base?
Conversation
The current help output of kakoune doesn't mention that a double dash (`--`) can be used to separate the files from options.
This special case was not handled in the `ParametersParser` and therefore did not respect the `--` separator for switches.
Adding the `WithCoord` flag will make the `ParametersParser` try to parse a special `+<line>:<column>` VIM-style switch.
This moves the parameter parsing for the `+<line>:<column>` switch of the commandline to the `ParametersParser`. The position switch now respects the `--` separator between switches and files. The manpage does not document the old behaviour of `+:[column]`. `+:[column]` works exactly like `+`. This is compatible with the previously documented behaviour for `+:`. The help message is does not mention that omitting the line or column moves the cursor to the last line or column. The explanation in the manpage should suffice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not too keen on this WithCoord change, the ParameterParser is used for parsing arguments to all commands in Kakoune, and it seems wrong to add a special case to it for this very specific argument type.
If we want to treat --
as "only file names after this" as vim does, I would prefer to look for "--" in the parameters and only run the parameter parser on arguments before it, adding arguments after it to the files vector.
Searching for Doing a simple Here's an example implementation: auto dd = find(params, `--`);
auto parser = ParametersParser{Range{params.begin(), dd}, param_desc};
if (dd != params.end())
++dd;
auto position = find_if(parser, [](auto &p){ return p.starts_with("+"); });
std::optional<BufferCoord> init_coord = std::nullopt;
Vector<StringView> positonals{parser.begin(), position};
if (position != parser.end())
{
positionals.insert(positionals.end(), position + 1, parser.end());
// parse position
init_coord = ...;
}
positionals.insert(positionals.end(), dd, params.end());
I find this significantly more complex than the @mawww Would you still prefer using something like my pseudo code above? |
Sorry for the delay in responding, life got busy, I guess both are not too satisfying, but I'd rather keep the coord handling out of the general code. So yeah, I'd prefer the version that does not support passing |
Problem
There is no consistent way to open an arbitrary filename using the
kak
commandline.Bad edgecases are:
--help
-
+
Current Behaviour
--help
from the command line.-
can be opened by adding a--
parameter to separate them from options.+
can be opened by adding a+0
to the end of the commandline. Only the last occurrence of a+
prefixed parameter is used for positioning within the buffer.This means that
kak -- test0 +test1 -test2 +0
will open buffers fortest0
,+test1
and-test2
this works around 2 and 3. Butkak -- --help +0
still shows the usage string.To fix problem 1 you'd currently have to generate an argument to the
-e
switch that includesedit -- "some file"
for each file you want to open. The generated kakoune script in turn would of course need to have the filenames escaped for kakoune...Expected Behaviour
--help
special case is just weird and I'd like it to be removed or respect the--
separator.--
should also prohibit any occurrence of a+
positioning argument afterwards.I want to be able to guarantee that
kak -- <FILE>
opens a file named<FILE>
and does nothing else with the argument.What did I do?
--help
special case. There's no nice way to make it work.ParametersParser
to accept a+<line>:<column>
switch according to it's current behaviour on the command line, except that it does respect the--
.+<line>:<col>
special handling inmain.cc
with the newParametersParser
switch.Other Sideeffects
The parsing of
+
and+:
is currently not doing what is documented. It's implementation currently uses agj
instead of age
command. This behaviour is fixed in the new implementation.Consideration
This is technically a breaking change to the commandline invocation of kakoune. Invocations using both
--
and+<line>:<column>
will break, though I've never seen it used like this.The usage string is slightly incorrect. The position switch needs to come before the
--
.This new ParametersParser support for a
+<line>:<col>
switch could now also be used for theedit
command. This would allow the commandline and the builtin command to use the same syntax for specifying the postition. I don't know how I would combine it withedit
's current line and column argument though, since I don't want a breaking change here.