phpcs-variable-analysis icon indicating copy to clipboard operation
phpcs-variable-analysis copied to clipboard

Feature: Allow variables on a per-file basis

Open rvock opened this issue 7 years ago • 10 comments

I like the idea of allowing some variables only in some files. Other linteres have a /* global */ special comment, where you can define variables which are passed into the file: https://eslint.org/docs/rules/no-undef https://www.jslint.com/help.html#global

I know phpcs-variable-analysis has an validUnusedVariableNames option. But this is for all files and can not be set on a per-file basis.

rvock avatar Aug 24 '18 07:08 rvock

First of all, this is a great idea. Being able to set per-file options would be amazing for a whole lot of reasons and I could make use of this feature myself.

That said, I kind of feel that this would be something better tackled at the level of phpcs itself, rather than a single sniff such as this.

Of course, features like this can take a while to be implemented upstream even if they are agreed upon, so we could try to add something beforehand. Since it would effectively be a very different kind of feature, I think it might be best to add it as a separate phpcs plugin; I don't think it would be particularly hard to allow it to work for all sniff options. If I have time I'll look into what that would take, but ideas and other attempts are welcome.

sirbrillig avatar Aug 24 '18 15:08 sirbrillig

PHPDoc has an @global, so maybe this is the proper way to deal with it. http://docs.phpdoc.org/references/phpdoc/tags/global.html

chybaDapi avatar Oct 31 '18 00:10 chybaDapi

I opened an issue upstream to discuss this possibility: https://github.com/squizlabs/PHP_CodeSniffer/issues/2424

sirbrillig avatar Feb 15 '19 01:02 sirbrillig

Is there any update on how this can be achieved? If I understand https://github.com/squizlabs/PHP_CodeSniffer/issues/2424 correctly that is not an option.

Personally I'd been fine with being able to specify validUnusedVariableNames for a single directory, but from what I understand that is not something that phpcs supports.

stefanfisk avatar Apr 29 '20 12:04 stefanfisk

This is the latest, I believe: https://github.com/squizlabs/PHP_CodeSniffer/issues/2126 You might want to suggest it there.

sirbrillig avatar May 01 '20 15:05 sirbrillig

@rvock

I wanted to suggest the same thing today as you did when I saw your issue. I don't know if this is still interesting for you, but I found a workaround.

I have read the mentioned issues squizlabs/PHP_CodeSniffer#2424 and squizlabs/PHP_CodeSniffer#2126 and then experimented a little. The problem was, as soon as the exceptions are defined, they also apply to all files that are subsequently checked. Then I simply tried to delete the relevant property in all other files. That worked, but it was also extremely impractical and inelegant. I then came up with the idea of ​​deleting the property in the same file in which I set it. That's working. :) CS seems to set a variable as soon as it is read. Exactly this behavior can be used. My workaround now looks like this:

<?php
// phpcs:set VariableAnalysis.CodeAnalysis.VariableAnalysis validUnusedVariableNames var1 var2 var3

<php code>

// phpcs:set VariableAnalysis.CodeAnalysis.VariableAnalysis validUnusedVariableNames

This is not perfect because the workaround takes an extra line at the end of the file, but I can live with that. ^^

LukeWCS avatar Jun 30 '21 15:06 LukeWCS

@LukeWCS Your suggestion for a work-around (set and reset within the same file) is exactly what is currently used in test case files in PHPCS itself to prevent settings changed for a test influencing another test. It can be fiddly as certain properties may have a default value and resetting it by unsetting it, would still influence the behaviour of the sniff for further files being scanned. So, the value should always be reset to the default value, which people may need to look up in the sniff.

I think the phpcs:set directive was originally only intended for those test cases. The issue you commented on upstream is me basically suggestion we change that behaviour and make the annotation more widely usable ;-)

jrfnl avatar Jul 01 '21 13:07 jrfnl

@jrfnl What do "test case files" mean?

Okay, that means with this workaround it must generally be considered whether a property has a default value. In this case this shouldn't be a problem, i.e. in relation to validUnusedVariableNames for VA.

It would of course be better if the CS team established a proper standard in order to be able to define properties per file. Also with regard to the parallel option.

LukeWCS avatar Jul 02 '21 11:07 LukeWCS

What do "test case files" mean?

@LukeWCS Sorry, that's how PHPCS itself (and external standards) are tested - with a test file (class) and a test case file with "fake" code. The test case file is given to PHPCS and then the test class checks if PHPCS throws the right warnings/errors on the right lines and such and fixes correctly.

jrfnl avatar Jul 02 '21 12:07 jrfnl

@jrfnl So a kind of self-test. Thanks for the answer.

LukeWCS avatar Jul 03 '21 07:07 LukeWCS