search

sirbrillig / phpcs-variable-analysis

Find undefined and unused variables with the PHP Codesniffer static analysis tool.
Other
136 stars 14 forks source link

Feature: Allow variables on a per-file basis #45

Open rvock opened 6 years ago

rvock commented 6 years ago

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.

sirbrillig commented 6 years ago

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.

chybaDapi commented 6 years ago

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

sirbrillig commented 5 years ago

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

stefanfisk commented 4 years ago

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.

sirbrillig commented 4 years ago

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

LukeWCS commented 3 years ago

@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. ^^

jrfnl commented 3 years ago

@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 ;-)

LukeWCS commented 3 years ago

@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.

jrfnl commented 3 years ago

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.

LukeWCS commented 3 years ago

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