Add an option validation on cli commands by aledeg · Pull Request #2278 · FreshRSS/FreshRSS

aledeg · 2019-03-17T10:33:43Z

If an option used on cli is not recognized, the command
aborts and displays an error message.
If the typed option is similar to one of the recognized
options, a hint is displayed.

At the moment, there is a limitation on long options.
Short options are not validated at the moment.

See #2046

Frenzie

Interesting idea, although given the fairly limited number of options I'm not entirely convinced that it's actually more helpful than the usage notice. :-)

Frenzie · 2019-03-17T10:41:26Z

cli/_cli.php

+	    if (0 === count($possibleOption)) {
+            fail(sprintf('FreshRSS error: unknown option: %s'), $unknownOption);
+        } else {
+            fail(sprintf('FreshRSS error: unknown option: %s. Do you mean %s?', $unknownOption, array_shift($possibleOption)));


I'd say "Did you mean" or perhaps "The most similar command/option/flag is:".

Side note, what if there are multiple results with a low Levenshtein distance?

Noted.
In the case of multiple results, the first one is displayed

aledeg · 2019-03-17T11:52:08Z

I can remove the the suggestion part. It was just an experiment.
But the validation part is interesting because if one of the options is inexistent, the following one are discarded. In that case, you could end up doing things that you don't want to do.
See related issue for example.

Frenzie · 2019-03-17T12:02:38Z

You could also consider printing both, so you'd get something like this:

Did you mean --bla

Usage notice: --bla --yada

aledeg · 2019-03-17T12:58:11Z

Considering how the cli is build, it won't be possible to have both

Frenzie · 2019-03-17T13:08:39Z

I think usage info is more useful personally. Any thoughts @Alkarex ?

aledeg · 2019-03-17T13:09:19Z

I can do that but that means a lot of changes

Frenzie · 2019-03-17T13:13:23Z

I don't think suggestion is very useful though. :-)

(At least not when there are fewer than 10 options.)

aledeg · 2019-03-17T13:28:13Z

@Frenzie I've changed the behavior to display wrong parameters AND the usage. Let me know what you think!

Frenzie

lgtm!

Frenzie · 2019-03-17T13:40:22Z

cli/_cli.php

+		return true;
+	}
+
+	fwrite(STDERR, sprintf("FreshRSS error: unknown options: %s\n", implode (', ', $unknownOptions)));


I suppose you could consider giving fail() an argument regarding whether to die or not (true by default), or create a non-die()ing duplicate like a warning() function. But that might be premature abstraction.

That's a good point. Maybe we should do that if we intend to add more warnings.

Alkarex · 2019-03-19T12:41:24Z

@aledeg Could you please fix the Travis white-space error? :-)

aledeg · 2019-03-19T13:21:16Z

@Alkarex. Will do!

If an option used on cli is not recognized, the command aborts and displays an error message. If the typed option is similar to one of the recognized options, a hint is displayed. At the moment, there is a limitation on long options. Short options are not validated at the moment. See FreshRSS#2046

aledeg · 2019-03-19T19:18:50Z

@Alkarex it's all good!

oupala · 2019-03-22T10:09:09Z

As far as I have tested this merge request, it fixes the bug mentioned in #2046.

Thanks @aledeg for fixing this.

I still have 2 remarks:

it is very strange to get syntax inconsistency in parameter name: --api_password seems not natural as we usually use dash or underscore, but not both of them, we should convert all parameter to dash syntax: --api-password
it is also very strange to see that installing FreshRSS on an already installed FreshRSS returns an error, following idempotency we should more return a 0 return code:

>php cli/do-install.php --default_user myuser --language fr --api-enabled --db-type sqlite
FreshRSS looks to be already installed! Please use `./cli/reconfigure.php` instead.
>echo $?
1

#2294 #2296 #2278 #2046 #2288 #2290 #2291 #2292 #2273 #2276 #2286 #2281 #2282

If an option used on cli is not recognized, the command aborts and displays an error message. If the typed option is similar to one of the recognized options, a hint is displayed. At the moment, there is a limitation on long options. Short options are not validated at the moment. See FreshRSS#2046

FreshRSS#2294 FreshRSS#2296 FreshRSS#2278 FreshRSS#2046 FreshRSS#2288 FreshRSS#2290 FreshRSS#2291 FreshRSS#2292 FreshRSS#2273 FreshRSS#2276 FreshRSS#2286 FreshRSS#2281 FreshRSS#2282

Frenzie reviewed Mar 17, 2019

View reviewed changes

Alkarex added this to the 1.14.0 milestone Mar 17, 2019

aledeg force-pushed the feature/cli-options branch from 42152ac to 8de7b2b Compare March 17, 2019 13:27

Frenzie approved these changes Mar 17, 2019

View reviewed changes

aledeg mentioned this pull request Mar 18, 2019

CLI should check parameter name, and exit in error when encountering an unknown parameter #2046

Closed

aledeg force-pushed the feature/cli-options branch from 8de7b2b to fea1119 Compare March 19, 2019 19:11

Alkarex merged commit 71b4226 into FreshRSS:dev Mar 19, 2019

aledeg deleted the feature/cli-options branch March 19, 2019 19:31

Alkarex added a commit that referenced this pull request Mar 23, 2019

Changelog

ae1da83

#2294 #2296 #2278 #2046 #2288 #2290 #2291 #2292 #2273 #2276 #2286 #2281 #2282

mdemoss pushed a commit to mdemoss/FreshRSS that referenced this pull request Mar 25, 2021

Changelog

8490479

FreshRSS#2294 FreshRSS#2296 FreshRSS#2278 FreshRSS#2046 FreshRSS#2288 FreshRSS#2290 FreshRSS#2291 FreshRSS#2292 FreshRSS#2273 FreshRSS#2276 FreshRSS#2286 FreshRSS#2281 FreshRSS#2282

Uh oh!

Conversation

aledeg commented Mar 17, 2019

Uh oh!

Frenzie left a comment

Choose a reason for hiding this comment

Uh oh!

Frenzie Mar 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aledeg Mar 17, 2019

Choose a reason for hiding this comment

Uh oh!

aledeg commented Mar 17, 2019

Uh oh!

Frenzie commented Mar 17, 2019

Uh oh!

aledeg commented Mar 17, 2019

Uh oh!

Frenzie commented Mar 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aledeg commented Mar 17, 2019

Uh oh!

Frenzie commented Mar 17, 2019

Uh oh!

aledeg commented Mar 17, 2019

Uh oh!

Frenzie left a comment

Choose a reason for hiding this comment

Uh oh!

Frenzie Mar 17, 2019

Choose a reason for hiding this comment

Uh oh!

aledeg Mar 17, 2019

Choose a reason for hiding this comment

Uh oh!

Alkarex commented Mar 19, 2019

Uh oh!

aledeg commented Mar 19, 2019

Uh oh!

aledeg commented Mar 19, 2019

Uh oh!

oupala commented Mar 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Frenzie Mar 17, 2019 •

edited

Loading

Frenzie commented Mar 17, 2019 •

edited

Loading