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

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

Open
rvock opened this issue Aug 24, 2018 · 10 comments
Open

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

rvock opened this issue Aug 24, 2018 · 10 comments

Comments

@rvock
Copy link

rvock commented Aug 24, 2018

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
Copy link
Owner

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
Copy link

chybaDapi commented Oct 31, 2018

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
Copy link
Owner

I opened an issue upstream to discuss this possibility: squizlabs/PHP_CodeSniffer#2424

@stefanfisk
Copy link

Is there any update on how this can be achieved? If I understand squizlabs/PHP_CodeSniffer#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
Copy link
Owner

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

@LukeWCS
Copy link

LukeWCS commented Jun 30, 2021

@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
Copy link
Collaborator

jrfnl commented Jul 1, 2021

@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
Copy link

LukeWCS commented Jul 2, 2021

@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
Copy link
Collaborator

jrfnl commented Jul 2, 2021

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
Copy link

LukeWCS commented Jul 3, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants