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

Allow use within PHP_CodeSniffer itself #215

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fredden
Copy link
Member

@fredden fredden commented Dec 5, 2023

Proposed Changes

While looking into changing the coding standard used for PHP_CodeSniffer itself, I noticed that this plug-in doesn't currently work in that scenario. This pull request allows this plug-in to be used within the PHP_CodeSniffer repository.

Related Issues

Related to PHPCSStandards/PHP_CodeSniffer#122 (review)

@jrfnl
Copy link
Member

jrfnl commented Dec 5, 2023

@fredden I'm not sure I see the point of this change ?

  1. PHPCS itself does not (currently) use outside standards for its own CS check, so making this change at this time is not needed.
  2. Making a change in this package for a potential future situation which is specific to only one potential user, while there are other options available to that user which will work just as well... doesn't seem to justify the maintenance burden of the change.

What am I missing ?

@jrfnl jrfnl added the triage label Dec 5, 2023
@fredden
Copy link
Member Author

fredden commented Dec 5, 2023

  1. PHPCS itself does not (currently) use outside standards for its own CS check, so making this change at this time is not needed.

Correct. I was working on changing that, as you'd suggested that in the future it would be good to use the PHPCSDevCS standard internally for PHP_CodeSniffer. That change is low priority, and involves a large change to the code-base, so is unlikely to happen in the short term. In order to avoid duplication, I installed PHPCSDevCS via Composer, and found this incompatibility.

  1. Making a change in this package for a potential future situation which is specific to only one potential user, while there are other options available to that user which will work just as well... doesn't seem to justify the maintenance burden of the change.

Making this change seemed cleaner and more robust than setting a hard-coded installed_paths in several places within PHP_CodeSniffer. Depending on how that would be handled there, anyone wanting to contribute to that project may also need to perform an additional manual set-up step. Note too that the PHPCSDevCS standard depends on at least one other standard via Composer which will need to be included in the installed_paths setting.

@jrfnl
Copy link
Member

jrfnl commented Dec 5, 2023

  1. PHPCS itself does not (currently) use outside standards for its own CS check, so making this change at this time is not needed.

Correct. I was working on changing that, as you'd suggested that in the future it would be good to use the PHPCSDevCS standard internally for PHP_CodeSniffer. That change is low priority, and involves a large change to the code-base, so is unlikely to happen in the short term. In order to avoid duplication, I installed PHPCSDevCS via Composer, and found this incompatibility.

  1. Making a change in this package for a potential future situation which is specific to only one potential user, while there are other options available to that user which will work just as well... doesn't seem to justify the maintenance burden of the change.

Making this change seemed cleaner and more robust than setting a hard-coded installed_paths in several places within PHP_CodeSniffer. Depending on how that would be handled there, anyone wanting to contribute to that project may also need to perform an additional manual set-up step. Note too that the PHPCSDevCS standard depends on at least one other standard via Composer which will need to be included in the installed_paths setting.

I understand what you are trying to do, but this is not the time. Let's focus on what's in front of us right now first.

It may still be years before that CS change in PHPCS itself happens, it may never happen. If anything, in my mind, it will likely be an iterative process with small changes to normalize the code a bit more over time, bit by bit, until it is at a point that a switch would be realistic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants