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

Config: revamp the help screen(s) #447

Merged
merged 1 commit into from
Apr 28, 2024
Merged

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Apr 13, 2024

👉🏻 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:

  • 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 defaults 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. See Config: read all CLI args before doing a deep exit #448.
  • 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 page before tagging the release which will contain this change.

Suggested changelog entry

The help screens have received a face-lift.

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Documentation improvement

Testing this PR

To test this PR manually:

  • Either check out the PR branch or download the 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).
  • Run the following commands to see all different outputs:
    • phpcs --colors -h
    • phpcbf --colors -?
    • phpcs --no-colors -h
    • phpcbf --no-colors --help
  • Additionally, run the above with variations of --report-width=# (passed on the command-line before the -h).
  • Lastly, view the output with different command-line colour schemes.

I'm in particularly looking for feedback on the choices made regarding:

  • the categorization of the options
  • the (improved) text of the option descriptions
  • the colours in regards to how they hold up when different colour schemes are used/against a light background.

Screenshots

New Help screen for PHPCS

image

New Help screen for PHPCBF

image

src/Util/Help.php Outdated Show resolved Hide resolved
src/Util/Help.php Outdated Show resolved Hide resolved
src/Util/Help.php Outdated Show resolved Hide resolved
src/Util/Help.php Outdated Show resolved Hide resolved
src/Util/Help.php Outdated Show resolved Hide resolved
src/Util/Help.php Outdated Show resolved Hide resolved
src/Util/Help.php Outdated Show resolved Hide resolved
src/Util/Help.php Outdated Show resolved Hide resolved
src/Util/Help.php Outdated Show resolved Hide resolved

'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".',
Copy link
Contributor

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?

Copy link
Member Author

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.

@mbomb007
Copy link

The PR changes --colors to be the default, but the help text is wrong regarding that.

@GaryJones
Copy link
Contributor

The PR changes --colors to be the default, but the help text is wrong regarding that.

I don't believe it does. From the original PR description:

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.

@mbomb007
Copy link

Okay, maybe I misunderstood what the change was when reading the code.

@jrfnl
Copy link
Member Author

jrfnl commented Apr 16, 2024

At the suggestion of @GaryJones - this is what the "before" looked like:

PHPCS:
image

PHPCBF:
image

@jrfnl jrfnl force-pushed the feature/config-revamp-help-screen branch from 652af67 to 45a6d33 Compare April 23, 2024 22:28
@jrfnl
Copy link
Member Author

jrfnl commented Apr 23, 2024

Rebased without (further) changes after the merge of #445 to drop the commits which belonged to #445. Marking as ready for review.

If there is no further feedback I will merge this in a couple of days.

@jrfnl jrfnl marked this pull request as ready for review April 23, 2024 22:30
src/Util/Help.php Outdated Show resolved Hide resolved
*/
private function getMaxWidth()
{
return max(self::MIN_WIDTH, $this->config->reportWidth);
Copy link

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)?

Copy link
Member Author

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() with max(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.
@jrfnl
Copy link
Member Author

jrfnl commented Apr 28, 2024

Rebased without changes, just squashed the commits together. Merging once the build passes.

@jrfnl jrfnl force-pushed the feature/config-revamp-help-screen branch from 68e4370 to b0f05e3 Compare April 28, 2024 10:40
@jrfnl jrfnl merged commit 42eafcf into master Apr 28, 2024
48 checks passed
@jrfnl jrfnl deleted the feature/config-revamp-help-screen branch April 28, 2024 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants