Dummy PR to test #17719 #17720
Closed
Dummy PR to test #17719 #17720
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Skipped packages/analyzer, there's too much going on there. The most interesting bit is the switching of abtest to use WorDBless so it can pick up the WP_Error definition it needs instead of trying to locally delcare it in its tests which results in a phpcs complaint about the redefined class. Also account for two files in the requirelist that had been moved. I'm tempted to swtich the requirelist to an "excludelist" so we can more easily exclude things like packages/analyzer and don't have to worry about tracking new files and renames of files that are supposed to be clean, but I'll leave that for a future PR.
The end goal is to have all files being fully checked by phpcs. The progress towards that goal is easier to check when it's "no files are left in bin/phpcs-excludelist.txt" than when it's "all files are covered by bin/phpcs-requirelist.js", and it's easier to find work to do when that work is "clean up a file in excludelist" than "clean up a file that's not in requirelist". This also makes things more straightforward when adding new files, as the default will now be to fully check the files rather than to not check them but have dangerfile.js try to complain about them. It also avoids files accidentally falling out of the requirelist if they're renamed and the requirelist isn't updated. Plus, the existing requirelist is inaccurate: it claims everything in packages/ is required, but in practice packages/analyzer/ has never passed. This change itself won't do anything directly to prevent that sort of thing in the future, but cleaning it up is necessary if we want to start having CI run phpcs over the required files.
This makes several changes to the custom scripts available via composer: * Most of the phpcs-running commands are renamed to be prefixed with "phpcs:" rather than "php:". The old names are kept as deprecated aliases in all but one case. * "php:lint" really should lint (`php -l`), not run phpcs. * Composer always puts vendor/bin/ first in the path, so there's no need to include that in all the script definitions. * Running phpcs via composer can hit composer's 300-second timeout. We can avoid that by instructing composer not to apply the timeout. * Commands can build on others, to avoid repeating mostly the same thing (e.g. the timeout disabling) many times. * We can add documentation for the scripts to the output of `composer list`.
anomiex
added
[Type] Bug
We seem to be moving away from Travis and to Github Actions. So, this moves linting and PHPCompatibility out of Travis and into a Github Action. This also adds running of the full PHPCS Standard over the non-excluded files, so we don't regress on them in the future as we have in the past.
anomiex
force-pushed
the
try/github-action-for-lint-and-phpcs
branch
from
ddc9e67
to
33d9dff
Compare
anomiex
force-pushed
the
dummy/test-pr-17719
branch
from
8d010a0
to
fd91dfe
Compare
--report=checkstyle does XML entity encoding. So let's try --report=emacs, which doesn't seem to do any escaping.
anomiex
force-pushed
the
dummy/test-pr-17719
branch
from
fd91dfe
to
48260e1
Compare
The anchoring was breaking it.
The full phpcs run will include syntax checking (Generic.PHP.Syntax) and all of the PHPCompatibility rules that we're already doing separately. So ignore them for that run to avoid duplicate messages.
anomiex
force-pushed
the
dummy/test-pr-17719
branch
from
48260e1
to
10bba77
Compare
There's a space before the `/>` in the `<error>` tag.
anomiex
force-pushed
the
dummy/test-pr-17719
branch
from
10bba77
to
24a8006
Compare
Copy the right magic from https://github.com/actions/cache/blob/main/examples.md#php---composer
anomiex
force-pushed
the
dummy/test-pr-17719
branch
from
24a8006
to
9ab9650
Compare
anomiex
force-pushed
the
dummy/test-pr-17719
branch
5 times, most recently
from
c65c799
to
1bd167b
Compare
Semi-arbitrarily chose 5.6, 7.0, 7.4, and 8.0. Note some lint errors will be detected by multiple versions, and will therefore show multiple copies of the error in Github. Similarly, errors caught by both a linter and PHPCompatibility will have both listed.
anomiex
force-pushed
the
dummy/test-pr-17719
branch
from
1bd167b
to
ea67be7
Compare
There should be no need to check deleted files.
Not planning on making this particular job required, but it might be nice for info.
…-for-lint-and-phpcs
And fix new issues in two files. Sigh.
If someone creates a branch with a name ending in `.php`, phpcs would try to check it. We don't want that.
…-for-lint-and-phpcs
Base automatically changed from
try/github-action-for-lint-and-phpcs
to
master
November 30, 2020 21:18
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes proposed in this Pull Request:
Do not merge this. It intentionally adds several broken PHP files to
test the workflow being added in PR #17719.
Jetpack product discussion
None.
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
Proposed changelog entry for your changes: