Skip to content
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

Command Line Parser Concept #6099

Merged
merged 67 commits into from
Feb 28, 2024
Merged

Conversation

KasimirCash
Copy link
Contributor

Changes proposed in this pull request:

Continuing on from #6036, parsing of command line options has now been cleaned up and wrapped into a CommandLineParser Class.
Options on commands are now represented by an Option object and are added via the calls to the addOption() and addRequiredOption() methods on CommandLineParser where they are given a name and their properties can be set.

We now have the following concepts associated with an option

  • Aliases: Options can be given short and long alias' via Option's constructor and a deprecated alias via the deprecatedAs() method.
  • Value Taken: The existing behaviour of each option either requiring a value, having an optional value or taking no value is maintained but is now set via one of; withValueRequired(), withValueOptional() or withValueNone(). For withValueOptional() a default value can also be set for when the option is used but given no value, e.g. an option with withOptionalValue('true') will be given the input 'true' if it is used like a flag.
  • Type Taken: Options now have a type they accept that is set via one of; typeOfString(), typeOfBool(), typeOfInt() or typeOfArrayOfString(). Inputs for that option must be castable to that type e.g. typeOfInt() must be all digits, typeOfBool() must be 'true'/'false', 'on'/'off', '1'/'0' etc. An error will be raised if the input is not castable to the wanted type.

Additionally, a default input value can be set as the final argument in addOption(). This will be given to the option as input if it does not have any user input. This allows us to have options that will always return a value.

User input is retrieved and parsed via the parse() method on CommandLineParser. parse() must be given a class string to use for constructing it's output, at the moment the we only ever pass stdClass::class to it so that we can utilise dynamic properties.
Calls to parse() return an object with any error messages generated inside an errors property, a usage message for the command under a usage property and any valid inputs cast to the correct type under a property with the same name as the option the input was given to.

Invalid option checking on short options has been improved. The short option check introduced in #6036 could return a false positive in a certain edge case, this no longer happens.

We now also handle an option being set multiple times by a user and so having multiple values by returning only the last valid input unless the option has an array type in which case we return all valid inputs as an array.

The only alteration to existing behaviour, other than the extra validation, is that options that address boolean values in config such as --allow-anonymous, --disable-update etc. have been made to optionally accept boolean values. This allows us to maintain current behaviour of those commands (a flag that implies true) while also allowing us to also give them an explicit true or false should we wish to. Meaning we can now turn these option off from the command line.

Overall this means that we will now have a single abstraction that handles usage messages, input errors, type checking and type casting, standardising them and so saving us from having to reimplement them inside every command. It also becomes trivial to add options to an existing command and puts things in a good position for any further expansion or refactoring of the command line tools.

How to test the feature manually:

All behaviour should remain the same as before with the exception of the above mentioned changes to flags addressing boolean values in config, which can be tested by giving one those options a value e.g. --disable-update=false and confirming config has been appropriately updated.

Pull request checklist:

  • clear commit messages
  • code manually tested
  • unit tests written
  • documentation updated

@Alkarex
Copy link
Member

Alkarex commented Feb 21, 2024

@KasimirCash I had a number of suggested changes, so I have made a PR to your PR :-)
Please check KasimirCash#1

  • More strongly typed, more object oriented
    • Example: $options = new ActualizeUserDefinition(); instead of $options = $parser->parse(ActualizeUserDefinition::class);
  • Addressed PHPStan ignore
  • Avoid some variable variables
  • Simplify and shorten a bit

@Alkarex
Copy link
Member

Alkarex commented Feb 26, 2024

Changes KasimirCash#1 merged here for easier feedback

@KasimirCash
Copy link
Contributor Author

Changes KasimirCash#1 merged here for easier feedback

Sorry about the delay. Okay, so I've had a read through this and everything looks really good to me.

@Alkarex
Copy link
Member

Alkarex commented Feb 27, 2024

I have just made some light renaming: 163ad9a
I was not so found of the many new classes used only once and defined locally (not in their own file), so I have used a bit of syntactic sugar and updated them to anonymous classes, which also helps with regard to the naming of the classes, which I have tried to make a bit more consistent.

Example:

$cliOptions = new class extends CliOptionsParser {
	public string $user;

	public function __construct() {
		$this->addRequiredOption('user', (new CliOption('user')));
		parent::__construct();
	}
};

instead of:

final class ActualizeUserDefinition extends CommandLineParser {
	public string $user;

	public function __construct() {
		$this->addRequiredOption('user', (new Option('user')));
		parent::__construct();
	}
}

$options = new ActualizeUserDefinition();

@Alkarex Alkarex merged commit 4b29e66 into FreshRSS:edge Feb 28, 2024
2 checks passed
@Alkarex
Copy link
Member

Alkarex commented Feb 28, 2024

Merged 🚀 Thanks again!

@KasimirCash
Copy link
Contributor Author

Thank you for the extensive feed back. It's been really valuable.

@KasimirCash KasimirCash deleted the validate-cli-input branch February 28, 2024 13:41
Comment on lines -15 to -16
if (preg_grep("/^$username$/i", $usernames)) {
fail('FreshRSS warning: username already exists “' . $username . '”', EXIT_CODE_ALREADY_EXISTS);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this test caused a regression, fixed in #6214

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI Command line interfaces in ./cli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants