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

Build/Travis/PHPCS: various minor improvements #1316

Merged
merged 4 commits into from
Mar 1, 2018

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Mar 1, 2018

Build: move WPCS's own ruleset to the root directory

When the WPCS native PHPCS ruleset was introduced in PR #590, this file was initially placed in the bin directory so as not to confuse users looking for the example custom ruleset.

By renaming the file to .phpcs.xml.dist - take note of the . -, which is possible since PHPCS 3.1, the file is no longer listed directly next to the example ruleset, so it should be fine to move it to the repo root directory.

[Update]: I'd originally forgotten to adjust the .gitattributes, .gitignore and Contributing files for this change. I've now updated those to match and amended the original commit to include those changes.

Build/PHPCS: add the <file> directive

As the WPCS own ruleset is now in the root directory, we can add the <file> directive.
This allows:

  • devs to run the CS check locally without command-line arguments
  • Travis to run with less fewer command-line arguments.

Build/PHPCS: run PHPCS against the highest stable PHP version

... for optimal results from the tokenizer.

Build/Travis: drop support for HHVM

This has been brought up numerous times previously, but at those times, the builds for HHVM were still passing, so no effort was needed to maintain support.

However... the builds against HHVM have not been passing for quite a while now, so now might be a good time to (finally) drop support for HHVM.

@ntwb
Copy link
Member

ntwb commented Mar 1, 2018

screen shot 2018-03-01 at 6 35 02 pm

I'd prefer not to including the preceding period in the filename rename, I'd much prefer it be renamed to phpcs.xml.dist,

  1. I think it is distinguishable in the directory listing per the attached screenshot

  2. With the addition and work carried out thus far on adding phpcs.xml.dist files to various repos including WordPress Core having standardized filenames where applicable should be maintained. It adds consistency across the many sources of documentation that get generated, for example, documentation sources can include here WPCS, DevHub and many of the WordPress focused blogs that write about developing WordPress.

@jrfnl
Copy link
Member Author

jrfnl commented Mar 1, 2018

With the addition and work carried out thus far on adding phpcs.xml.dist files to various repos including WordPress Core having standardized filenames where applicable should be maintained. It adds consistency across the many sources of documentation that get generated, for example, documentation sources can include here WPCS, DevHub and many of the WordPress focused blogs that write about developing WordPress.

For what it's worth:

  1. Both using phpcs.xml.dist as well as .phpcs.xml.dist is supported by PHPCS (3.1+) and are therefore a standard practice.
  2. Most tool configuration files in this industry are prefixed with a ., so adding the . to the phpcs.xml.dist filename would actually comply more with industry standards.

@ntwb
Copy link
Member

ntwb commented Mar 1, 2018

I ❤️ "dotfiles", I've even my own dotfiles repo.

The preceding period means that the file will be hidden from a standard directory listing ls vs ls -a and I don't think this file should be hidden, which is a subjective decision on my part.

I'd rather the single change that is being made here in this PR rather than multiple changes elsewhere to maintain a nomenclature consistency throughout the WordPress ecosystem

@jrfnl
Copy link
Member Author

jrfnl commented Mar 1, 2018

I don't think this file should be hidden, which is a subjective decision on my part.

Why ?

The WPCS PHPCS ruleset is intended just and only for the WPCS repo and is not suitable for, nor intended for other WordPress related projects.

I'd rather the single change that is being made here in this PR rather than multiple changes elsewhere to maintain a nomenclature consistency throughout the WordPress ecosystem

Ok, so here my English is failing me. What do you mean by this ?
Are you implying that if we make this change, the rest of the WP ecosystem should follow our example ?
Are you saying that consistency across the WP ecosystem is more important than practical choices based on the reality of individual repos ?

jrfnl added 4 commits March 1, 2018 10:06
This file was initially placed in the `bin` directory so as not to confuse users looking for the example custom ruleset.

By renaming the file to `.phpcs.xml.dist` - take note of the `.` -, which is possible since PHPCS 3.1, the file is no longer listed directly next to the example ruleset, so it should be fine to move it to the repo root directory.
As the WPCS own ruleset is now in the root directory, we can add the `<file>` directive.
This allows:
- devs to run the CS check locally without command-line arguments
- Travis to run with less command-line arguments.
... for optimal results from the tokenizer.
@jrfnl jrfnl force-pushed the feature/move-wpcs-phpcs-file-to-root branch from ad52761 to 760bbbb Compare March 1, 2018 09:07
@GaryJones
Copy link
Member

FWIW, I'm easy on the naming.

We are a Standard after all, and can use the latest PHPCS for ourselves, so there doesn't seem to be a reason not be a good example of how to do things.

WP Core, and likely whatever other repos Stephen refers to, use at least PHPCS 3.1, so they can choose, based on their own setup, whether they want to use the leading dot or not. I don't think there a massive amount of value (or hinderance by including a "(or .phpcs.xml.dist)" in tutorials) for consistency here.

Knowing that there is an option on naming (and let's not forget, you could call it stephen.xml if you were happy to pass it as a command line argument) is part of learning about PHPCS, so if the naming of our config file gets the inquisitive cogs whirring, irrespective of whether other repos in the WP ecosystem copy us, then that's a good thing.

Aside: What bugs me more is that PHPUnit doesn't support the leading .!

@GaryJones GaryJones merged commit 988b73f into develop Mar 1, 2018
@GaryJones GaryJones deleted the feature/move-wpcs-phpcs-file-to-root branch March 1, 2018 15:21
@jrfnl
Copy link
Member Author

jrfnl commented Mar 1, 2018

@GaryJones I'd say: add your voice to the open issue at PHPUnit: sebastianbergmann/phpunit#3030

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.

4 participants