-
-
Notifications
You must be signed in to change notification settings - Fork 57
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
Config: revamp the help screen(s) #447
Conversation
|
||
'config-explain' => [ | ||
'text' => 'Default values for a selection of options can be stored in a user-specific CodeSniffer.conf configuration file.'."\n" | ||
.'This applies to the following options: "default_standard", "report_format", "tab_width", "encoding", "severity", "error_severity", "warning_severity", "show_warnings", "report_width", "show_progress", "quiet", "colors", "cache", "parallel".', |
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.
Does this long string of options have some logic for the order they are in? Would alphabetical be better?
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.
The order is based on the order used in the Config class (restoreDefaults()
method).
I'm happy to change it, but am not sure alphabetic would be the best choice as that would pull the set of "severity" related options - severity
, error_severity
, warning_severity
, show_warnings
- apart.
Suggestions for an improved order are welcome.
The PR changes |
I don't believe it does. From the original PR description:
|
Okay, maybe I misunderstood what the change was when reading the code. |
At the suggestion of @GaryJones - this is what the "before" looked like: |
652af67
to
45a6d33
Compare
*/ | ||
private function getMaxWidth() | ||
{ | ||
return max(self::MIN_WIDTH, $this->config->reportWidth); |
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.
Can $this->config->reportWidth
change during execution?
If not, then instead of this method, would it be worth creating another property, $maxWidth
and setting this in ::__construct()
with max(self::MIN_WIDTH, $this->config->reportWidth)
?
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.
Can
$this->config->reportWidth
change during execution?
Not once this class gets instantiated (and PHPCS exits straight after).
If not, then instead of this method, would it be worth creating another property,
$maxWidth
and setting this in::__construct()
withmax(self::MIN_WIDTH, $this->config->reportWidth)
?
I had that in an earlier draft of the class, but ended up with the method as I found it more descriptive for the other width calculations to have all these calculations in methods close together (so the logic is more comprehensible).
This PR attempts to make the help information more informative and to improve the readability and findability of options. This should be seen as a first step to address user concerns about difficulty in finding the options they are looking for and understanding how certain options work, like seen in the recent months in tickets 10/294/419, 248, 322, 415 and 434. The format for the new screens is inspired by similar help screens as currently in use in various other typical CLI tools, like PHPUnit and PHPStan. This includes the choice for the use of colours and which colours to use. The option descriptions are based on the previously available option descriptions with some improvements where I deemed those appropriate. I've elected to keep the descriptions short though as this is a help screen, not a tutorial. Notes: * I've chosen to move the logic to generate the help screen to a separate (internal) class. This new class is fully covered by tests, including various QA tests which function as error-prevention when new options would be added. * The output, by default, is not coloured, as PHPCS default to `--no-colors`. To get coloured output, either ensure the `colors` option is saved as `1` in the user-specific configuration using `--config-set colors 1` (making PHPCS default to `--colors`) or pass `--colors` on the command line. Note: if `--colors` is passed on the command line _after the `-h` argument, it will have no effect. This is a symptom of how the CLI argument processing currently works and is considered as out of scope of this issue. * The output will respect the `report-width` setting (which defaults to `auto`, i.e. width of the current screen), as long as the `report-width` is 60 columns or more. If the `report-width` is set to below 60 columns, a width of 60 will be used anyway to allow for displaying the texts. * `colors` and `report-width` settings as saved to a `CodeSniffer.conf` file via `--config-set` will be respected when displaying the help screens. * `colors` and `report-width` settings provided via a custom ruleset have no effect on the help screens. Todo: - [ ] Update Wiki [Usage](https://github.com/phpcsstandards/PHP_CodeSniffer/wiki/Usage) page before tagging the release which will contain this change.
Rebased without changes, just squashed the commits together. Merging once the build passes. |
68e4370
to
b0f05e3
Compare
👉🏻 This PR depends on PR #445 .
Description
This PR attempts to make the help information more informative and to improve the readability and findability of options.
This should be seen as a first step to address user concerns about difficulty in finding the options they are looking for and understanding how certain options work, like seen in the recent months in tickets #10/#294/#419, #248, #322, #415 and #434.
The format for the new screens is inspired by similar help screens as currently in use in various other typical CLI tools, like PHPUnit and PHPStan.
This includes the choice for the use of colours and which colours to use.
The option descriptions are based on the previously available option descriptions with some improvements where I deemed those appropriate.
I've elected to keep the descriptions short though as this is a help screen, not a tutorial.
Notes:
This new class is fully covered by tests, including various QA tests which function as error-prevention when new options would be added.
--no-colors
. To get coloured output, either ensure thecolors
option is saved as1
in the user-specific configuration using--config-set colors 1
(making PHPCS default to--colors
) or pass--colors
on the command line.Note: if
--colors
is passed on the command line _after the-h
argument, it will have no effect. This is a symptom of how the CLI argument processing currently works and is considered as out of scope of this issue. See Config: read all CLI args before doing a deep exit #448.report-width
setting (which defaults toauto
, i.e. width of the current screen), as long as thereport-width
is 60 columns or more.If the
report-width
is set to below 60 columns, a width of 60 will be used anyway to allow for displaying the texts.colors
andreport-width
settings as saved to aCodeSniffer.conf
file via--config-set
will be respected when displaying the help screens.colors
andreport-width
settings provided via a custom ruleset have no effect on the help screens.Todo:
Suggested changelog entry
The help screens have received a face-lift.
Types of changes
Testing this PR
To test this PR manually:
phpcs.phar
/phpcbf.phar
files generated as build artifacts in the GH Actions workflows (these can be found on the "Summary" page of the latest "Test" workflow run for this PR).phpcs --colors -h
phpcbf --colors -?
phpcs --no-colors -h
phpcbf --no-colors --help
--report-width=#
(passed on the command-line before the-h
).I'm in particularly looking for feedback on the choices made regarding:
Screenshots
New Help screen for PHPCS
New Help screen for PHPCBF