Small refactor of 'else if' statements in hyprctl #6585
Conversation
Yaraslaut
changed the title
| {"/decorations", 1} | ||
| }; | ||
| for (const auto& [cmd, minArgs] : commands) { | ||
| if(fullRequest.contains(cmd)) |
There was a problem hiding this comment.
hm, this might bug out if the rest of the request has the thing. Maybe substring only the part after the first / and before the first space?
There was a problem hiding this comment.
I think that i do not change any behavior of parsing in this pr.
for safer way of handling cli option i would go directly with something like https://github.com/contour-terminal/contour/blob/121d7ade938ac99a18ff0b4f650254778baddafd/src/contour/ContourApp.cpp#L297
and then just
_syntax = parameterDefinition();
optional<CLI::flag_store> flagsOpt = CLI::parse(_syntax.value(), argc, argv);
There was a problem hiding this comment.
you didn't but when already changing it why not improve?
There was a problem hiding this comment.
my feeling is that this is not a good place for input validation, especially inside for loop during check for the flags
There was a problem hiding this comment.
it's not input validation - it's making sure you choose the right value in case of e.g.
/notify blah blah /decorations
There was a problem hiding this comment.
If /notify blah blah /decorations is not ill-formed then logic is not right, since only /notify command will be handled
There was a problem hiding this comment.
no, the /decorations is a part of the parameter. Notify for example takes a string, so the user can put whatever.
vaxerski
force-pushed
the
main
branch
2 times, most recently
from
1184395
to
b16fb97
Compare
Describe your PR, what does it fix/add?
Small refactor of else if statements inside hyperctl
Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)
Also, some of the outputs of hyperctl does not have new lines in the end, namely all calls to
logfunction, if you are interested i can refactor it for a bit and introduce usage ofstd::printlnIs it ready for merging, or does it need work?
ready