-
Notifications
You must be signed in to change notification settings - Fork 4
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
Feedback on initial setup #1
Comments
I don't want to write lower than that in context of this project. Yes, adoption might depend on trying to support as low versions as possible, but this existing depends on me not being annoyed by it. If someone needs lower versions support I'll happily consider getting paid for it. 😊
Done.
Eh, it's a bad fit for my preferred workflow. This works for me for the moment.
What? Why?
There is probably half a dozen way to go about it. I decided to just give couple fastest to get up and checking. Not intending to re-document PHPCS here.
Done, needed namespace to get this going.
I know, did that while having a fight with it about getting sniff name right. Is this a problem particularly? It kinda serves as self-documenting example for someone reusing in downstream ruleset.
Done?.. I pillaged from WPCS, PHPCS ruleset docs seem to make no mention of this...
Done. I started with it, but had a fight with whole thing getting the sniff name right.
Wasn't me. Done.
Wasn't me. I get your point but don't know how to fix this right away.
Wasn't me. Done.
Wasn't me. Done.
The two are what spec covers. I will add more if I encounter that SonarLint (which I use as reference implementation) does anything with them.
I am not sure. Spec says to favor constructs like these, so leaving it out for now.
Done.
It already does this, just that there are different kinds of tokens we want to do different things with? It could probably be improved somewhat, but I don't see it right away.
Again, don't know how to fix it right away...
As in adding namespace to ruleset (done as above) or something else? Thanks a lot for extensive feedback! :) |
Ah, found the leftover namespace. :) |
Sorry, I meant |
Hi @Rarst, as discussed on Slack, please find some initial feedback below:
Note: all current feedback is just and only based on a quick look at the codebase. I haven't run any tests with the repo (yet).
composer.json
:dev
dependencies with a higher minimum PHP version as their own project (and Composer warns against doing that).autoload
section. This is known to cause issues with PHPCS.no-dev
requirement, not adev
requirement. Otherwise users may still get an older PHPCS version if they also use other external standards which have a lower minimum PHPCS version..gitignore
:Readme:
require --dev
as this sniff is normally not a production requirement for other packages.installed_paths
and use the ruleset by name. There are numerous bugs known for when standards are not installed.Ruleset:
<autoload>
in the ruleset. PHPCS >= 3.1.0 should handle this fine as is. Though maybe not if the standard is not installed... hmm.... well, see my remark above in that case.And you don't need to include the sniff in the ruleset, all sniffs in the
Sniffs
directory of a standard will automatically be included.Have a look at WPCS to see an example.
Sniff code:
process()
making the cognitive load for other sniff devs higher (breaks the principle of least astonishment).$method
: you are presuming a code style. Best practice sniffs should be code style agnostic.While not very common, this will break on:
addError()
function already hassprintf()
build in, so no need to do it yourself. Just pass the replacements as an array in the fourth parameter.self::class
is a bad idea as:I'd suggest using a plain string like
TooHigh
.You could even consider setting two levels, like
maxCognitiveComplexity
andrecommendedCognitiveComplexity
, throw an error for the first, a warning for the second and have separate error code for each, likeTooHigh
andHigh
Analyzer code:
$booleanOperators
- you are missing three operators. Please consider using the PHPCS nativeTokens::$booleanOperators
array instead.$operatorChainBreaks
- what about null coalesce ?isset()
instead ofin_array()
.computeFor...()
andcontinue
if not one of your targets.You are now also passing whitespace and comment tokens off for processing which is just plain inefficient.
$nextToken
inisIncrementingToken()
is not code style agnostic.Tests:
Anyways, hope this helps.
The text was updated successfully, but these errors were encountered: