Conversation
If this package should be tested against PHPCS + WPCS, then these development dependencies should be available locally, and not rely on a contributor having it available globally, with what might be out of date versions. WPCS is stable enough to facilitate a version of `dev-master`, but I've stuck with the latest tag for now. The DealerDirect Composer plugin allows the automatic registration of code standards that include the `type` of `phpcodesniffer-standard`, and is recommended by WPCS.
Makes local running of PHPCS more user-friendly
| case 'flag': | ||
| if ( 'Y' == strtoupper( $response ) ) | ||
| $assoc_args[$spec_arg['name']] = true; | ||
| if ( 'Y' == strtoupper( $response ) ) { |
There was a problem hiding this comment.
Also not addressed, would be obvious changes of comparison from equality to identity, like this.
|
Hi @GaryJones Thanks for PR. But we had a discussion about this. We are having a several repositories, I think it is very hard. I am very sorry, but I want to close it. |
|
Hi @miya0001, #3703 is from January 2017. Since then though, @danielbachhuber himself opened #4058 in May 2017, to which this PR directly (partially) addresses (including only the code in this repo), so I'd strongly infer that this PR is still relevant. |
|
Thanks :)
Ah, OK. @wp-cli/committers Any thoughts? |
|
@GaryJones The PR touches two different (albeit related) topics: 1. "Setting up WPCS as a dev dependency" and 2. "Code style fixes that can be automatically applied". I'd like to merge everything related to 1. for now, but want to have an issue created for 2. first (or add comments to #4058 if this makes sense), to discuss how to go about this. As @danielbachhuber had pointed out in #4058 , he'd like to have 1 PR per sniff, to re-enable them in a controlled fashion. Also, this would need to be somewhat coordinated with the bundled commands, as the testing suite setup is sporadically "applied" (through the scaffolding tools) to the commands as well. So, re-enabling a sniff would mean it needs to be dealt with across all official repos. |
I can pull into a new branch and create a PR.
As I updated my original post, this doesn't make sense from a practical point of view. |
Yes, please do, so that we can at least merge this change for now.
Hmm, yes, I see. Let's put this on hold until @danielbachhuber is back online to discuss with him. Although I would love to have cleaner code, I find it to be difficult already to step through the project's blame as it is. I don't want to merge another blanket change across the entire codebase unless we're all on the same page about it. |
@GaryJones I'm amenable to more sniffs per PR (e.g. two or three or a half dozen). My goal in setting a lower bound is to make the PR more easily reviewable by a human. If completing the issue takes a dozen PRs over a couple of weeks, I don't see the time commitment as a blocker for the approach. One massive PR with lots of churn makes it very hard for a human to review all of the changes being applied. |
|
May I recommend that instead of small batches of sniffs across all files, that the approach is all of the sniffs fixed in single / small batches of files? That should reduce the workload needed before accepting a PR before it goes stale. Ultimately, the goal is not to review the code such as "did it meet this part of the code standard, did it meet that part of the code standard" (which would be derived from arbitrarily grouping sniffs), but "does this file now meet the code standards without breaking functionality". If that's acceptable, then I'm happy to make a start on it. |
Sure, this is acceptable to me.
Correct. |
|
My branches are done, but I'll wait until #4339 can be merged before doing the PRs, to save them all failing Travis build and having to be re-triggered. |
Partially addresses #4058.
See the individual commits for more info.
While I appreciate that the desire in #4058 was to do one commit per error (not even one per Sniff), that really doesn't make sense, as some fixes work together to produce the correct outcome i.e. one auto-fix would put something in a new line, but another would fix the indentation; applying them in separate commits doesn't achieve atomic commits that meet acceptable standards that make for easy reviewing.
As such, I did one commit of a couple of items that threw errors even with the exclusions still in place, another that removed exclusions for auto-fixable items, and another for a non-auto-fixable item.
The remaining exclusions are non-trivial to fix i.e. renaming files, renaming public methods / functions etc.
I'd encourage this to be merged, and handle the other exclusions in separate commits / PRs, since they may well need more discussion.
Also not handled is any change in CI, since I'm not familiar with how this repo is handling that.