The phpcs.xml file for Typed Data has been copied from Rules and still has <ruleset name="rules">. Although fairly trivial, it would be nice to change this to typed_data. Secondly, there is one coding standards fault in all branch test runs
1 coding standards message
README.txt
line 60 Line exceeds 80 characters; contains 82 characters
The phpcs.xml allows long lines for README.md (as that is what Rules has), so it would be simple to change this to README.* then we'll have a clean standards pass.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | 3093178-4.interdiff-2-to-4.txt | 1.43 KB | jonathan1055 |
| #4 | 3093178-4.improve-phpcs-xml.patch | 1.66 KB | jonathan1055 |
Comments
Comment #2
jonathan1055 commentedPatch to do the above. Also as a precaution (like we did in Rules) added lines to make sure that all Drupal and DrupalPractice rules are included, before any custom tailoring.
Comment #3
tr commentedSeems like that file hasn't been touched for a while, so it's good to give is a once-over and bring it up to date.
The changes you made in #2 are good. Looking at the rest of the file, here are some other things I noticed - can you take a look at these too?
Maybe also remove
Generic.Arrays.DisallowLongArraySyntax? This is already covered by the core checks I believe.Also, not sure why
Drupal.Commenting.DocComment.MissingShortwas put in there long ago, but if some documentation is missing I would like to know so we can add it and not just ignore it. I suspect this is a problem only for test cases, which tend not to have much in the way of documentation ... Regardless, it can probably be removed then a new issue opened to clean up the missing short descriptions.It used to be that core didn't make an exception for annotations, which I assume is why
Drupal.NamingConventions.ValidVariableName.LowerCamelNamewas added. But is that still true and do we still need this?What does
<arg value="s"/>do?I guess excluding vendor and config is OK, even though that's not needed for this project. Or should we just remove those?
Comment #4
jonathan1055 commentedYes it is, so it can go from here. In fact if we have a long array, we get two messages in the phpcs output:
As the whole file was copied from Rules, there must have been a time when this was needed to be excluded from Rules checking. Typed Data has no faults if this exclusion goes.
No, we don't get any failures if this exclude pattern is removed.
It prints the full sniff name in the report, which does not affect testing on d.o. but does help when running phpcs locally. If we have a problem with a sniff, it is useful to have the name immediately to hand, for searching in Coder for what it is doing, or use it to exclude it. I've added a comment, and also added
<arg value="-colors"/>to get the report in color by default.As those folders do not exist in the typed_data project we might as well remove those lines.
One further question:
There are no failures if we remove this exclusion, but Rules does need this exclusion I think. Do you have a view on empty lines after comments? I think we should allow them, as it can make it clearer when commenting a large block of code, e.g. one main general comment applies to a large chunk of code, which needs a blank line after it. Then shorter specific comments apply to the code lines immediately following them. So I'm happy to leave this exclusion in, and from the fact that you did not make reference to it it appears you are ok with it too.
Comment #6
tr commentedThanks for looking into those things and providing the answers.
I totally agree. I don't think it should be common, but as you described there are times when this is really the best way to comment the code and I don't want to discourage good commenting.
Your patch looks great and is ready to go. Committed #4. Thanks!