[go: nahoru, domu]

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

Retire phpcs-diff #446

Merged
merged 6 commits into from
Jul 1, 2024
Merged

Retire phpcs-diff #446

merged 6 commits into from
Jul 1, 2024

Conversation

eason9487
Copy link
Member
@eason9487 eason9487 commented Jun 28, 2024

Changes proposed in this Pull Request:

This PR retires phpcs-diff and add the phpcs workflow.

Checks:

  • Does your code follow the WordPress coding standards?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Detailed test instructions:

  1. npm run lint:php to see if the check can pass
  2. View the check result of the phpcs workflow
  3. View the annotations of linting errors added by the phpcs workflow
    image

Changelog entry

@eason9487 eason9487 self-assigned this Jun 28, 2024
@github-actions github-actions bot added the changelog: dev Developer-facing only change. label Jun 28, 2024
@eason9487 eason9487 requested a review from a team June 28, 2024 09:33
@@ -69,8 +70,10 @@ public function set_path_props() {
* Set server props
*/
public function set_server_props() {
// phpcs:disable WordPress.Security.ValidatedSanitizedInput.MissingUnslash, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we can sanitize the input here instead of ignoring the line?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since this file is used for PHPUnit testing, I thought it would be okay to skip the checks.

I revisited its logic once and found that after the rewrite it didn't have to skip checking. Adjusted in db107f7.

Copy link
Contributor
@puntope puntope left a comment

Choose a reason for hiding this comment

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

Thanks @eason9487 for opening this PR

I tested it and works

Screenshot 2024-06-28 at 18 16 25

I left one comment regarding the handle of the PHPCS exceptions instead of ignoring them if possible, in this PR or maybe create an Issue for it.

Approved tho since it's not a blocker because it was not introduced in this PR.

@eason9487 eason9487 merged commit 360c351 into trunk Jul 1, 2024
6 checks passed
@eason9487 eason9487 deleted the dev/retire-phpcs-diff branch July 1, 2024 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: dev Developer-facing only change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants