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

Use ANSI color codes and Unicode characters on Windows as well #662

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MatmaRex
Copy link
Contributor

@MatmaRex MatmaRex commented Nov 9, 2024

Description

Use ANSI color codes and Unicode characters on Windows as well, when possible. See commit messages for details.

Suggested changelog entry

  • Output now has fewer redundant ANSI color codes.
  • On Windows, when using the --colors option, the output now has more colors.
  • On Windows, when using PHP 7.1+, the output now has more Unicode characters.

Related issues/external references

(none)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
    • This change is only breaking for integrators, not for external standards or end-users.
  • Documentation improvement

PR checklist

  • I have checked there is no other PR open for the same change.
  • I have read the Contribution Guidelines.
  • I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
  • I have added tests to cover my changes.
  • I have verified that the code complies with the projects coding standards.
  • [Required for new sniffs] I have added XML documentation for the sniff.

@jrfnl
Copy link
Member

jrfnl commented Nov 10, 2024

@MatmaRex Thanks for the PR.

However, similar to the other PR, I need to ask you to please either provide a reproduction scenario, screenshots or tests to demonstrate what you are trying to solve. Preferably all three of these should be provided with any PR which changes things in the UI.

I can take a guess what you are trying to solve, but my guess may be wrong, so without any of the above, I can't even begin to properly evaluate the PR.

PHP enables this by default since PHP 7.2. https://www.php.net/manual/en/function.sapi-windows-vt100-support.php

PHPCS 3.x still has a minimum supported PHP version of PHP 5.4, so we cannot rely on features from PHP 7.2.
Please test your changes with PHP < 7.2 and see what breaks (and yes, things will break).

I could tentatively earmark this PR for PHPCS 4.0 , which will have a PHP 7.2 minimum, but that still doesn't negate my remarks above.

@MatmaRex
Copy link
Contributor Author

@MatmaRex Thanks for the PR.

However, similar to the other PR, I need to ask you to please either provide a reproduction scenario, screenshots or tests to demonstrate what you are trying to solve. Preferably all three of these should be provided with any PR which changes things in the UI.

I can take a guess what you are trying to solve, but my guess may be wrong, so without any of the above, I can't even begin to properly evaluate the PR.

While using Windows 10 or newer, and PHP 7.2 or newer, run PHPCS with --colors e.g. on this file: https://github.com/MatmaRex/phpcs-pull-654-demo/blob/master/demo.php

Before After
before after

Note:

  • On PHP 7.2 and newer, the inline "\n" is now highlighted using a different color. This is the same behavior as you'd see on Linux.
  • On PHP older than 7.2, or on Windows older than 10, the --colors mode is already not working correctly. As a result of this change a few additional artifacts appear.

PHP enables this by default since PHP 7.2. https://www.php.net/manual/en/function.sapi-windows-vt100-support.php

PHPCS 3.x still has a minimum supported PHP version of PHP 5.4, so we cannot rely on features from PHP 7.2. Please test your changes with PHP < 7.2 and see what breaks (and yes, things will break).

I could tentatively earmark this PR for PHPCS 4.0 , which will have a PHP 7.2 minimum, but that still doesn't negate my remarks above.

The --colors mode is already broken before my patch, as shown in the screenshots above (tested on 7.1). The --no-colors mode is unaffected by my patch. I believe nothing new breaks.

If you really want to support these old systems, the only thing to do would be to disable --colors entirely. I could write a patch for that, if you promise you won't ask me for any test cases for it. I'm not sure what the point would be since they wouldn't run in CI anyway.

@jrfnl
Copy link
Member

jrfnl commented Nov 10, 2024

@MatmaRex

The --no-colors mode is unaffected by my patch. I believe nothing new breaks.

Given the following simple file:

<?php

    // This is a space indented line.
    $a = 10;
	// This is a tab indented line.

And running the following command on the Windows 10 command line:

phpcs ./phpcs-662.php --report=code --tab-width=4 --no-colors --standard=generic

☝️ Take note of the use of --report=code as that's the report which is being adjusted in this PR (amongst other things). Also note that this is in no-colors mode.

On PHP 8.4, everything looks swell:

Before After
image image

Not so much on PHP 7.0....:

Before After
image image

Which only goes to demonstrate why I insist on tests and will continue to do so...

If you really want to support these old systems, the only thing to do would be to disable --colors entirely. I could write a patch for that, if you promise you won't ask me for any test cases for it. I'm not sure what the point would be since they wouldn't run in CI anyway.

Proper tests will run in CI and should have caught the above. If not, the tests would not be good enough.

@MatmaRex
Copy link
Contributor Author

Oh, right, the "visible" characters are still applied in --no-colors mode. Thanks for pointing that out.

The encoding issue is interesting – it happens with PHP 7.0, but not with PHP 7.1, which I tested with.

Proper tests will run in CI and should have caught the above. If not, the tests would not be good enough.

I'm not very familiar with GitHub's CI – can you point out which of the jobs run on Windows?

@jrfnl
Copy link
Member

jrfnl commented Nov 11, 2024

Oh, right, the "visible" characters are still applied in --no-colors mode. Thanks for pointing that out.

The encoding issue is interesting – it happens with PHP 7.0, but not with PHP 7.1, which I tested with.

Might just be that PHP added support for the Windows ANSI codes in PHP 7.1 and only added the function related to it in PHP 7.2.

Proper tests will run in CI and should have caught the above. If not, the tests would not be good enough.

I'm not very familiar with GitHub's CI – can you point out which of the jobs run on Windows?

At this time, there are no runs on Windows as there are no tests covering code which has special conditions for Windows. As soon as such tests would be added, the Windows OS would have to be added to the test matrix/workflow. (No worries about that, I can do that).

@MatmaRex
Copy link
Contributor Author

Might just be that PHP added support for the Windows ANSI codes in PHP 7.1 and only added the function related to it in PHP 7.2.

The ANSI codes and the Unicode characters actually appear unrelated. I found a PHP doc page that documents the latter as a change in PHP 7.1. https://www.php.net/manual/en/migration71.windows-support.php

At this time, there are no runs on Windows as there are no tests covering code which has special conditions for Windows. As soon as such tests would be added, the Windows OS would have to be added to the test matrix/workflow. (No worries about that, I can do that).

I guess I can try adding some. Probably in yet another pull request.

This reduces the size of the output, and lessens weird effects
of the ANSI color code characters on text wrapping, when the
result of this method is used in a sniff error message
(e.g. see LanguageConstructSpacingSniff).
Windows' console supports ANSI escape sequences since Windows 10.
https://learn.microsoft.com/en-us/windows/console/console-virtual-terminal-sequences

PHP enables this by default since PHP 7.2.
https://www.php.net/manual/en/function.sapi-windows-vt100-support.php
On older PHP versions, the output with --colors will not be correct,
but this is already the case when using this option.

PHP CodeSniffer special-cased Windows in this code in 2014
(bfd095d).
This workaround is no longer needed today in 2024.

Note that the --colors option was already supported on Windows.
This only affects inline highlighting in error and debug messages.
@MatmaRex
Copy link
Contributor Author

I extended the changes here slightly, adding two additional commits (see commit messages for details):

@MatmaRex MatmaRex changed the title Use ANSI color codes on Windows as well Use ANSI color codes and Unicode characters on Windows as well Nov 17, 2024
…PHP 7.1+

PHP 7.1 fixed problems with printing Unicode characters to Windows
console that forced us to avoid using them.
https://www.php.net/manual/en/migration71.windows-support.php
@MatmaRex
Copy link
Contributor Author

Sorry about the noise, I didn't realize at first that PHPUnit 4.8 (used on PHP 5.4) does not support the @requires PHP < 7.1 syntax. And thanks for enabling automatic test runs for me :)

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.

2 participants