-
-
Notifications
You must be signed in to change notification settings - Fork 521
Add tests for code handling STDIN for the sniffs FileName and I18nTextDomainFixer #2512
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
Conversation
He @rodrigoprimo, thanks for this PR!
The Reason for that is threefold:
Does that help your understanding of the
Nice solution, though I do think there are alternative solutions which don't require the extra file. Alternative 1: use the real use PHP_CodeSniffer\Config;
$config = new Config();
$config->standards = ['WordPress'];
$config->sniffs = ['WordPress.Files.Filename']; Not great for performance of the tests, but should work. Alternative 2: use the PHPCSUtils 1.1.0 use PHPCSUtils\BackCompat\Helper;
use PHPCSUtils\TestUtils\ConfigDouble;
$config = new ConfigDouble();
Helper::setConfigData('installed_paths', dirname(dirname(__DIR__)), true, $config);
$config->standards = ['WordPress'];
$config->sniffs = ['WordPress.Files.Filename']; What do you think of those possibilities ? |
Thanks for all the information and the PR review, @jrfnl!
I can confirm that alternative 1 works indeed, as I tested it while trying to figure out why doing the same with
This seems to be the best solution, and I don't see a problem with waiting until |
Works for me 👍 |
PR #2532 bumps PHPCSUtils to 1.1.0, so once that is merged, this PR could be updated. |
Thanks for the ping, @jrfnl! I rebased this PR and added two new commits, one for each sniff test class, updating the STDIN test to use PHPCSUtils 1.1.0 I did this so that each commit can be squashed into the corresponding original commit. Let me know if you prefer a different approach. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rodrigoprimo Thanks for the update - feel free to squash those commits into the original commits already.
I've verified the tests work, despite the --sniffs
directive not being respected in the tests until PHPCS 4.0 (see PHPCSStandards/PHP_CodeSniffer#996), so all good from me.
The sniff bails early when handling STDIN. This commit adds a test to cover that part of the sniff code.
The sniff bails early when handling a plugin header passed via STDIN. This commit adds a test to cover that part of the sniff code.
Thanks for the squashing @rodrigoprimo . Merging now. |
This PR adds a test to cover code handling STDIN for the two sniffs that check it:
WordPress.Files.FileName
andWordPress.Utils.I18nTextDomainFixer
. This is based on a related suggestion from @jrfnl for a PHPCS sniff: PHPCSStandards/PHP_CodeSniffer#681 (review).I'm not sure if I missed something, but I could not make the
ConfigDouble
class work passing the standard and sniff names as parameters. I had to create a custom ruleset file and include the sniff passing its path. I believe this happens because theConfigDouble
class erases the config data, and thus,installed_paths
is empty.