Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
It would be nice if we refactored the module code in accordance with the Drupal 8 coding standards.
This mainly includes the following:
- Add a description of files, functions, classes and methods.
- The correct naming of classes and variables.
- Indents and line breaks.
What do you think about it?
Comment | File | Size | Author |
---|---|---|---|
#18 | coding-standards-3018715-18.patch | 209.19 KB | WalkingDexter |
Issue fork simple_sitemap-3018715
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commentedI am not religious about coding standards but I was under the impression that the 3 examples you mentioned are already in place. Well not all methods have their phpdoc comment, but all API methods should by now.
So not sure were you see the issue, maybe you can create a patch for one of the classes so I can see where my coding deviates from the standards.
Also it would be great if you could justify changes by pointing me to the relevant Drupal coding standard.
Comment #3
WalkingDexter CreditAttribution: WalkingDexter at Initlab commentedPatch for example.
Comment #4
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commentedThis patch is totally fine and these changes make sense. If you feel like it, go ahead and just push changes to 3.x-dev.
You also mentioned indents and line breaks, where do you see the problem? I believe you do not make code blocks within a method, whereas I like to separate logical chunks with line breaks - I believe both is acceptable; if you agree, we can leave it as is.
Feel free to push any non-breaking improvements to 3.x - it's your call whether to create an issue or just push. I don't mind reverting a change if I disagree so it's alright.
Comment #5
thallesFollow the patch!
Comment #6
thallesComment #8
WalkingDexter CreditAttribution: WalkingDexter at Initlab commented@thalles thank you for contributing! I made a few changes and committed.
Comment #10
WalkingDexter CreditAttribution: WalkingDexter at Initlab commentedTo understand what else needs to be fixed
https://pareview.sh/pareview/https-git.drupal.org-project-simple_sitemap...
Comment #11
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedPlease review!
Comment #12
rokzabukovec CreditAttribution: rokzabukovec at Agiledrop - Your Trusted Drupal Teammates commentedThe patch from #11 needed to be rerolled and I fixed all of the remaining coding standards issues.
Please review.
Comment #13
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commentedThanks @rokzabukovec; would you mind creating a merge request? That will make the review process much smoother.
Comment #15
rokzabukovec CreditAttribution: rokzabukovec at Agiledrop - Your Trusted Drupal Teammates commentedNo problem @gbyte. I opened a merge request so you can review the patch. I'm using the code sniffer tool with the Drupal standard set as a guideline. In this standard, every method should have a docblock and every parameter and return statement should have a description. It's a lot of change so you are going to probably need to change some descriptions, but these guidelines should be followed by every module or at least it's a good practice. That way the IDE can recognise the methods and offer some help when working with a module.
This should be at least a good starting point to make the module better.
Comment #16
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commentedWe've made plenty of changes in the past week, the MR will have to be rerolled.
Comment #17
WalkingDexter CreditAttribution: WalkingDexter at Initlab commentedComment #18
WalkingDexter CreditAttribution: WalkingDexter at Initlab commentedPatch with a complete code refactoring in accordance with the current Drupal coding standards. I didn’t fix some points, as they relate to description of update hooks and interface translation. @gbyte please review after tests succeeded.
P.S. I was unable to create a new branch (for 4.x) in the current issue fork. Therefore, I am sending a patch.
Comment #20
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commentedGreat work! I changed a bunch of the method descriptions and caught a missing import that led to exceptions.
@WalkingDexter
What about interface translation? There is #3238405: Make new configuration entities translatable, any chance you could check the codebase for problems in regards to it? I am e.g not sure if calls to $entity->label() are supposed to be run through t(). Thanks in advance.
Comment #21
WalkingDexter CreditAttribution: WalkingDexter at Initlab commented@gbyte
To check standards, I use the PHP_CodeSniffer tool and the ruleset from the Coder project. There is also an online version, but it doesn't work yet. Also note that the PHP_CodeSniffer integrates with PhpStorm.
To avoid reopening this issue, please use the above tools.
I checked out the code again locally in the 4.x branch. The result is as follows:
Interface translation problems are points related to the t() function. This can be partially solved by #3238405: Make new configuration entities translatable. I think that other points also need to be fixed.
Comment #22
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commented@WalkingDexter Feel free to look into this issue and fix the remaining warnings.
I'll try my best, but cannot promise to adhere to some of these standards - I find commit-level checking that forces you to break up lines kills programming flow big time.
Comment #23
WalkingDexter CreditAttribution: WalkingDexter at Initlab commentedThe main points have been fixed for now.