Skip to content

Add a phpcs.xml.dist file for PHP file formatting standards#35

Merged
schlessera merged 15 commits into
mainfrom
add/phpcs-file
Oct 27, 2022
Merged

Add a phpcs.xml.dist file for PHP file formatting standards#35
schlessera merged 15 commits into
mainfrom
add/phpcs-file

Conversation

@danielbachhuber

Copy link
Copy Markdown
Member

No description provided.

@danielbachhuber danielbachhuber added this to the 2.0.1 milestone Aug 12, 2022
@danielbachhuber danielbachhuber self-assigned this Aug 12, 2022
@schlessera

Copy link
Copy Markdown
Member

What is the purpose of this ruleset? To lint the PHP logic added to this repo specifically (like the maintenance commands), or to lint the entire bundle?
I don't think linting the entire bundle is useful, it will just block commits for no reason. Also, it would mean keeping the package-specific exclusions in merged sync with all packages.
The ruleset should instead point to the .maintenance folder only, where the PHP files specific to this repository are. It should then only include the rule exclusions that are needed for that part of the code.

@danielbachhuber

Copy link
Copy Markdown
Member Author

What is the purpose of this ruleset? To lint the PHP logic added to this repo specifically (like the maintenance commands), or to lint the entire bundle?

@schlessera The former: linting the maintenance commands.

The ruleset should instead point to the .maintenance folder only, where the PHP files specific to this repository are. It should then only include the rule exclusions that are needed for that part of the code.

My bad. I copied the phpcs.xml.dist from wp-cli/wp-cli because I thought we might want to keep them in sync in some fashion.

To make sure we're on the same page, can you annotate the PR with the changes you'd suggest?

Comment thread phpcs.xml.dist Outdated
Comment thread phpcs.xml.dist Outdated
Comment thread phpcs.xml.dist Outdated
Comment thread phpcs.xml.dist Outdated
Comment thread phpcs.xml.dist Outdated
Comment thread phpcs.xml.dist Outdated
danielbachhuber and others added 7 commits August 16, 2022 12:45
Co-authored-by: Alain Schlesser <alain.schlesser@gmail.com>
Co-authored-by: Alain Schlesser <alain.schlesser@gmail.com>
Co-authored-by: Alain Schlesser <alain.schlesser@gmail.com>
Co-authored-by: Alain Schlesser <alain.schlesser@gmail.com>
Co-authored-by: Alain Schlesser <alain.schlesser@gmail.com>
Co-authored-by: Alain Schlesser <alain.schlesser@gmail.com>
@danielbachhuber

Copy link
Copy Markdown
Member Author

@schlessera It doesn't look like composer phpcs works as expected locally...

$ composer phpcs
> run-phpcs-tests
sh: run-phpcs-tests: command not found
Script run-phpcs-tests handling the phpcs event returned with error code 127

Thoughts on how I should continue to iterate?

Also, should I avoid cloning all of the package repos in the job?

image

@schlessera

Copy link
Copy Markdown
Member

run-phpcs-tests is part of wp-cli/wp-cli-tests, so you'd need to pull that in as a development dependency. Not sure what that will produce given the symlinky madness in this package, you'll have to try and see. The wp-cli/wp-cli-tests also provides the main PHPCS ruleset you're extending, so you'd need it either way for PHPCS to work correctly.

Regarding the cloning, I guess it would make sense to have the action go through the entire flow. I'm just surprised that GitHub itself has trouble cloning these publicly accessible packages...?!

@danielbachhuber

Copy link
Copy Markdown
Member Author

@schlessera This is fixed up now.

@schlessera schlessera merged commit 6b4d558 into main Oct 27, 2022
@schlessera schlessera deleted the add/phpcs-file branch October 27, 2022 15:13
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