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

Respect all sniffs when reviewing PHP_CodeSniffer itself #122

Merged
merged 1 commit into from Dec 5, 2023

Conversation

fredden
Copy link
Member

@fredden fredden commented Dec 4, 2023

Description

This is a re-creation of squizlabs/PHP_CodeSniffer#3914

While working on another task, I noticed that warnings are ignored by default. This seems unintentional. Some discussion regarding this has taken place in squizlabs/PHP_CodeSniffer#3914

Suggested changelog entry

Respect warnings as well as errors from sniffs within the coding standard for PHP_CodeSniffer itself.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

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 verified that the code complies with the projects coding standards.

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fredden Thanks for porting this over. In its current state (with only one remark), I'm okay to accept this.

Note: further changes to the ruleset used by PHPCS are not my current priority. If anything, I'll likely want to standardize on something closer to PHPCSDevCS at some point in the future.

* @link https://www.php.net/language.constants.predefined PHP Manual on magic constants
* @see https://www.php.net/language.constants.predefined PHP Manual on magic constants
Copy link
Member

@jrfnl jrfnl Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather keep this at the correct tag (@link) than change it just to satisfy an outdated sniff. Could you please add an exception instead ?

@jrfnl
Copy link
Member

jrfnl commented Dec 4, 2023

P.S.: feel free to squash the commits to one commit when updating the PR.

@jrfnl jrfnl merged commit 890b579 into PHPCSStandards:master Dec 5, 2023
32 checks passed
@jrfnl
Copy link
Member

jrfnl commented Dec 5, 2023

Thanks @fredden !

FYI: I'm not adding a changelog entry as this is not a user-facing change.

@fredden
Copy link
Member Author

fredden commented Dec 5, 2023

If anything, I'll likely want to standardize on something closer to PHPCSDevCS at some point in the future.

I made a start on this. I got to the point where the standard is installed and runs successfully. The next step involves changing nearly every file in the repository. I've put it down for now. Would a draft pull request be valuable? I can trivially open this as a reminder / TODO item if you'd like.

@jrfnl
Copy link
Member

jrfnl commented Dec 5, 2023

If anything, I'll likely want to standardize on something closer to PHPCSDevCS at some point in the future.

I made a start on this. I got to the point where the standard is installed and runs successfully. The next step involves changing nearly every file in the repository. I've put it down for now. Would a draft pull request be valuable? I can trivially open this as a reminder / TODO item if you'd like.

Sorry, while I appreciate what you are trying to do, this is definitely not a priority at this moment and not something I want to spend time on at this time.

An open issue to discuss a plan to get there is one thing, an open PR, draft or not, will just be closed as noise at this time.

@fredden
Copy link
Member Author

fredden commented Dec 5, 2023

I agree regarding priorities. I know you have your own way of tracking backlog items; I trust that this is somewhere in that system.

For future reference, the work started here includes 09d13e3 and PHPCSStandards/composer-installer#215.

@jrfnl
Copy link
Member

jrfnl commented Dec 5, 2023

I agree regarding priorities. I know you have your own way of tracking backlog items; I trust that this is somewhere in that system.

I will be opening issues for most of those ideas/plans/backlog items, #105, #106, #120 and some older issues ported over from the Squizlabs repo are just the start of that, but 3.8.0 needs to get released first. The rest can wait and is just a distraction at this moment.

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.

None yet

2 participants