Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This is part of this meta issue: Make flag module pass coder review
In this issue, we aim just to fix the coding standards and not any doc block changes.
Comment | File | Size | Author |
---|---|---|---|
#8 | 2245601.8.flag_.coding-standards.patch | 75.33 KB | joachim |
#1 | flag-fix_coding_standards-2245601-1.patch | 96.46 KB | cs_shadow |
Comments
Comment #1
cs_shadow CreditAttribution: cs_shadow commentedThis patch most of the coding standard related issues. Most of the remaining issues are related to missing doc blocks, invalid/missing type hinting or missing scope identifiers which will be fixed as separate issues.
Comment #2
joachim CreditAttribution: joachim commentedYouch. That's going to take a while to go through!
Comment #3
joachim CreditAttribution: joachim commentedThat belongs in a bug report.
Comment #4
joachim CreditAttribution: joachim commentedThat colon should stay.
I don't think there's any convention for these separator markers -- in fact I think core frowns on them. I prefer without a full stop as it's a title.
I'm not convinced that a blank line after each switch case improves readability.
That's part of a direct paste from Views export. That shouldn't be changed unless the Views export changes.
Weird formatting here!
This is old code but I can see a reason for keeping this terminal comma.
Don't take my test debug statements away!
Comment #5
cs_shadow CreditAttribution: cs_shadow commented@joachim, I'll update the patch but before that I wanted to clarify few things:
Reg #2.2, Do you want the separation markers to be removed?
Reg #2.3, Its just that Core does it this way, so shouldn't follow those standards?
Reg #2.7, Do we really want our debug statements in production code?
I agree with the other things you pointed out and will fix them soon.
Comment #6
joachim CreditAttribution: joachim commented> I'll update the patch
Don't do that yet -- I'm halfway through converting it to a sequential patch.
> Reg #2.2, Do you want the separation markers to be removed?
I quite like them :)
> Reg #2.3, Its just that Core does it this way, so shouldn't follow those standards?
You're right, the coding standards have a space after each case in the example.
> Reg #2.7, Do we really want our debug statements in production code?
In tests, I'd say yes! They save time when updating or adding to tests!
Comment #7
cs_shadow CreditAttribution: cs_shadow commentedOh, that's good. I was just about to start to work on the patch. Would have been a lot of duplicate work.
Comment #8
joachim CreditAttribution: joachim commentedHere's most of your changes, but grouped into smaller commits in a sequential patch.
To apply this, use 'git apply' in a feature branch: it'll create a string of commits.
One thing I've removed is the changes to documentation indentation, this sort of thing:
I know the docs standards say to use indentation like that, but I hate it!!! My text editor puts me on odd-numbered columns when I hit tab, and this standard means the first character of documentation can't be reached by tabbing. Not going to happen in any of my modules, sorry (unless someone can help me configure my text editor to handle this!!!).
Comment #9
cs_shadow CreditAttribution: cs_shadow commentedChanges are much more clear with this sequential patch. Looks good. Do you think we can commit this so that i can start with the documentation issues?
Comment #10
joachim CreditAttribution: joachim commentedCommitted! Thanks for working on this.
git commit -m "Issue #2245601 by cs_shadow, joachim: Fixed coding standards errors." --author="cs_shadow "
BTW, the way to make sequential patches like that is:
- make a local branch for your work (which is a good idea anyway)
- make your commits
- do 'git format-patch --stdout' to get a sequential diff
Comment #12
cs_shadow CreditAttribution: cs_shadow commentedThanks @joachim. #10 was really helpful.