Conversation
- Extended DocParser to parse 'alias' metadata from PHPdoc parameters - Updated Subcommand to resolve aliases to canonical names before validation - Modified Configurator to support single-dash short arguments (e.g., -w, -n=5) - Added comprehensive unit tests for alias resolution - Added Behat feature tests for argument aliases Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
- Store the original DocParser instance in Subcommand to access YAML metadata - Fix access level visibility issue with $synopsis property - Add debug logging for alias resolution - Verify aliases work correctly with manual testing Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
- Simplify alias resolution logic with two-pass approach - Add documentation for single-letter short argument limitation - Improve code clarity and maintainability Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
YAML parsers interpret 'n', 'N', 'y', 'Y' as boolean values (false/true). This caused single-letter aliases like 'n' to be parsed as false and then converted to empty strings. Added special handling to convert boolean false back to 'n' and boolean true to 'y', along with proper type conversion and empty string filtering. Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
github-actions
bot
added
command:cli
This comment was marked as resolved.
This comment was marked as resolved.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…tation - Renamed all test methods in ArgAliasTest.php from camelCase to snake_case to match existing test conventions in the codebase - Enhanced documentation for YAML boolean value handling to explain the behavior more clearly, including the note that 'n'/'N' become 'n' and 'y'/'Y' become 'y' - Added new test case for YAML boolean handling to verify correct behavior - Mentioned that users can quote aliases in YAML to avoid boolean interpretation Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for argument aliases, a valuable feature for command-line tools. The changes span across argument parsing, doc comment parsing, and command dispatching, and are accompanied by a comprehensive set of new unit and feature tests. The implementation is logical and covers various use cases for aliases. I have one suggestion regarding a potential performance issue in DocParser.php where parsing aliases could be made more efficient. Other than that, the changes look good.
| public function get_arg_aliases() { | ||
| $aliases = []; | ||
| $bits = explode( "\n", $this->doc_comment ); | ||
|
|
||
| foreach ( $bits as $bit ) { | ||
| // Skip empty lines and separator lines | ||
| $bit = trim( $bit ); | ||
| if ( empty( $bit ) || '---' === $bit ) { | ||
| continue; | ||
| } | ||
|
|
||
| // Match parameter definitions: | ||
| // - [--param=<value>] | ||
| // - --param=<value> | ||
| // - [--param] | ||
| // - --param | ||
| if ( preg_match( '/^\[?--([a-z-_0-9]+)/', $bit, $matches ) ) { | ||
| $param_name = $matches[1]; | ||
| $param_args = $this->get_param_or_flag_args( $param_name ); | ||
|
|
||
| if ( $param_args && isset( $param_args['alias'] ) ) { | ||
| $param_aliases = (array) $param_args['alias']; | ||
| foreach ( $param_aliases as $alias ) { | ||
| // Handle YAML boolean values (YAML 1.1 spec interprets certain | ||
| // values as booleans: y/Y/yes/YES/n/N/no/NO/true/false/on/off) | ||
| // For single-letter aliases, we preserve the intent by converting | ||
| // boolean values back to their likely string representations. | ||
| // Note: This means 'n' and 'N' both become 'n', and 'y' and 'Y' become 'y'. | ||
| // Users can avoid this by quoting the alias in YAML: alias: 'N' | ||
| if ( false === $alias ) { | ||
| $alias = 'n'; | ||
| } elseif ( true === $alias ) { | ||
| $alias = 'y'; | ||
| } | ||
|
|
||
| // Convert to string if not already | ||
| $alias = (string) $alias; | ||
|
|
||
| // Remove leading dashes if present | ||
| $alias = ltrim( $alias, '-' ); | ||
|
|
||
| // Skip empty aliases | ||
| if ( '' === $alias ) { | ||
| continue; | ||
| } | ||
|
|
||
| $aliases[ $alias ] = $param_name; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return $aliases; | ||
| } |
There was a problem hiding this comment.
The implementation of get_arg_aliases() has a potential performance issue. It iterates over each line of the doc comment, and for each line that appears to be a parameter, it calls get_param_or_flag_args(). This helper method in turn calls get_arg_or_param_args(), which also iterates over all lines of the doc comment from the beginning. This results in a nested loop over the doc comment lines, leading to O(n²) complexity where n is the number of lines. For commands with many parameters and extensive documentation, this could introduce a performance bottleneck during command loading.
A more efficient approach would be to parse the doc comment in a single pass, possibly using a state machine to identify parameter definitions and their associated YAML blocks. This would avoid the nested iteration and improve performance.
Implementation Plan for Argument Aliases
Understand the existing codebase structure for argument handling
Extend DocParser to support parsing argument aliases from PHPdoc
Implement alias resolution logic
Handle short-form aliases (single character with
-)Add comprehensive tests
Run all tests and fix any issues
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.