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

Closed
wants to merge 1 commit into 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.

@Potherca
Copy link
Member

@jrfnl I think we might want to either close this MR or, if we leave it open, create a "Nice to have later" milestone. Thoughts?

@jrfnl
Copy link
Member

jrfnl commented Sep 10, 2024

@jrfnl I think we might want to either close this MR or, if we leave it open, create a "Nice to have later" milestone. Thoughts?

Unless and until there is a decision in PHPCS itself to start using external standards for its own CS check, there is no need for this PR. And even if such a decision were taken, I'm not sure it makes sense to handle that via this plugin when the use-case is based on only one user.

I think closing makes sense. We can revisit this if and when the above would come into play in the future.

@Potherca Potherca closed this Sep 13, 2024
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.

3 participants