Make Breakpoint module pass Coder Review

according to http://qa.drupal.org/8.x-status#tabset-tab-4 breakpoint already passes ... am I missing something?

Thanks for that link @bleen18. I never knew that link existed. What do you think of creating an issue that we can work on together bringing down the criticals and warnings?

...isnt that what #1518116: [meta] Make Core pass Coder Review is?

But that issue is postponed on no decision about #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines. See issue #1512434-45: Make Aggregator module pass Coder Review for latest reference about why it is postponed.

Also note that #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines has had no progress in last two months, so #1518116: [meta] Make Core pass Coder Review is effectively postponed until whenever.

If one wants to make any progress on making core comply with coding and documentation standards, apparently one has to do so in separate incremental issues rather than these meta issues. Although I must also note that the coding standards fix for the Help module #1533244: Make help module pass Coder Review was committed this past week (after much teeth pulling and attempting to make Coder module work for D8).

Based on the learnings from #1533244: Make help module pass Coder Review issue, I have made fixes, related to coding standards, in Breakpoint module. As per learning there, I have ignored following issues,

- Missing comment for @return statement
- Standard simpletest functions getInfo(), setUp(), and tearDown() don't get doxygen comments
- No scope modifier specified for function getInfo() in class files

Attaching a Patch here which will fix the remaining issues. I am also attaching Coder output coder_issues_breakpoint_2012_10_22.txt so that we can see the list of all issues.

Please review and let me know what else can be done.

Thanks for the patch @amitgoyal. I hope to get time to review this later today.

The following is a detailed review of the patch in #5 based upon the patch itself, the Coder issues report and then applying the patch locally and reading through each of the code files visually inspecting for any other missing coding standards issues.

1) Each of the changes in the current patch in #5 are correct from a coding standards perspective.

2) This module is missing a breakpoint.install file. At minimum, we need to create one in this patch and move breakpoint_enable() to that that new file. I also noticed that this function was highlighted with a sniff in the Coder report. That is good. See http://api.drupal.org/api/drupal/core!modules!system!system.api.php/func... for more information.

Not required for a coding standards patch, but when it is moved, it would be helpful to add @see's for _breakpoint_theme_enabled() and _breakpoint_modules_enabled().

We also should open a follow up issue in the breakpoint.module issue queue about whether there also should be breakpoint_disable(), breakpoint_install() and breakpoint_unitstall() functions added to that file as well.

3) The Coder report does not point out the inconsistent use of @param and @return directives in breakpoint_themes_enabled() and breakpoint_themes_disabled(). The are both hook implementations. I believe that either the @param directives should be removed or @return directives should be added. At present, the documentation does not conform with #1354: [Obsolete] API documentation and comment standards.

The same observation also applies to breakpoint_modules_enabled() and breakpoint_modules_uninstalled() pair.

4) I am surprised that Coder review does not highlight that each of the four variables passed into the four functions in 3) do not specify that they must be arrays. I believe this is the right issue to add array in the function declarations. Hence, I would make the change like 'function breakpoint_themes_enabled(array $theme_list) {'.

5) I am not sure what the Coder review is checking for a docblock. However, it ideally should sniff that there is an issue in the one-line summary for _breakpoint_theme_enabled(). The one line summary needs to start with an active verb. Hence, we should change 'Import' to 'Imports'.

I would also change the function declaration to 'function _breakpoint_theme_enabled(array $theme_list) {'.

6) The thoughts in 5) also apply to _breakpoint_modules_enabled() and _breakpoint_import_media_queries() ['Imports' and fourth variable $media_queries].

7) Reading the docblock for _breakpoint_import_media_queries() and in particular, the explanation for $source_type, I see that there is an issue that needs to be addressed. It concerns incomplete documentation of namespaced objects. See #1487760: [policy, no patch] Decide on documentation standards for namespaced items for more information.

Can we open up an issue in Coder to create a sniff to catch this type of violation of documentation standards?

Applying this standard, I believe the explanation for $source_type should be changed to:

 *   Either \Drupal\breakpoint\Breakpoint::SOURCE_TYPE_THEME or
 *   \Drupal\breakpoint\Breakpoint::SOURCE_TYPE_MODULE.

The same type of change is needed in _breakpoint_import_breakpoint_groups(), _breakpoint_delete_breakpoints(), _breakpoint_delete_breakpoint_groups(), and _breakpoint_group_create_or_load() function docblocks.

8) We need to change the verb tense on first line of most docblocks in breakpoint.module. This should be a sniff from a Coder review, which is not present. Also there are a number of places where array should be added to the function declaration of variables.

9) In _breakpoint_import_breakpoint_groups(), there is a throw new \Exception(). Yet there is no @throws directive in the docblock. This needs to be added here. This issue is also in a couple of other functions further down, like breakpoint_get_theme_media_queries(). My understanding is that the @throw directive should go after @return (if any) with a preceding blank line (like how @param and @return are separated).

We should open up an issue in Coder to sniff out this type of documentation violation.

10) Issue #1487760: [policy, no patch] Decide on documentation standards for namespaced items also applies to explanation of @return in breakpoint_group_load(). My understanding is that this line should be changed to:

 * @return \Drupal\breakpoint\BreakpointGroup|false

Note the leading slash '\'. Can this be added to whatever is opened to address #7 as well?

11) My understanding is that @todo directives should follow @see directives which is not the case in the same docblock. Can we open an issue in Coder to sniff this type of issue as well? It is not highlighted in the Coder report.

12) Same thought as 10) also applies to @return directive in _breakpoint_group_create_or_load().

This concludes my observations from reading through breakpoint.module file. I am going to save this comment now and continue the review for the module's class files and tests in another comment.

I am temporarily moving this to documentation component so that we can properly categorize it.

@jhodgdon Can you add a component category for the breakpoint.module? It will be moved there once that is added.

Also @jhodgdon is welcome to add any comments regarding the review in #7. Perhaps there are thoughts there that she as the committer of the eventual RTBC patch would prefer that are not included?

I created issue #1820034: Identify incorrect @param/@return directives in implements hook_*() docblocks to follow up on the review in #7 item 3).

I also have updated the issue summary so we have somewhere to track follow-up issues specific to this sub-issue. Shortly, I will be adding the same table to the mega issue so that others can be aware of common follow-up issues as well.

I created issue #1820082: Ensure @param array type used in function definition (as appropriate) to follow up on #7 item 4) and added it to the issue summary.

Adding a follow up issue table.

#7 thanks, do you know of any links to doc pages, so I won't make the same mistake for my next patch?

Is the report generated by the coder module or drupalcs?

Updated issue summary.

Added a follow-up issue #1820116: Sniff for incorrect namespaced objects in docblock @ directives.

Hi @attkis. I am not sure what portion of #7 you are asking about. Could you please clarify?

To learn more about how the report was generated, please ask @amitgoyal. I cannot generate one myself, but I believe it is coming from sniffs out of the Coder Sniffer sub-module of the Coder module.

From working with @amitgoyal in the Help module sub-issue, he posted that he had completed an initial port of Coder 7.2 to D8. I am not sure if that was done in a sandbox or simply as a patch. See #1816302: Code Review 7.x-1.2 to 8.x-1.x Patch for further information.

@attkis, if you roll a follow-up patch for #7, I would be happy to review the result. I am in the midst of opening follow-up issues for all of #7. Afterwards, I plan to do a further review of the Breakpoint classes and Tests. #7 only included the top level files in the module. However, I suspect we can incorporate any changes from that into what you might produce in the interim.

Also, please let me know if you have any other code patches that you would like me to review like #7 before they get committed to core.

I opened up issue #1820116: Sniff for incorrect namespaced objects in docblock @ directives to follow up on #7 item 7). I also added it to both this and the meta issue summaries.

@Lars - Thanks for the detailed review, but I am not sure if I need to make any changes there.

For example, 4th point in #7,
"Hence, I would make the change like 'function breakpoint_themes_enabled(array $theme_list) {'.",

it's already been specified in the function comment/doc that it is of type array "@param array $theme_list". So I don't think that we should change it.

I believe that firstly we should try to fix the relevant issues reported by Coder module and then work on additional enhancements, if any, in different issues.

Thanks for the #14 comment @amitgoyal. As I stated at the start of #7 in item 1), everything in your patch is correct and addresses what currently is reported by the Coder report. The breakpoint_enable() presence in the wrong file is caught by the Coder report and definitely needs to be added to the patch in this issue.

I added the rest of #7 to cover what else should be part of an ideal Coder review. For instance, all of the active verb tenses should be fixed, whether in this issue or another one.

My goal is simply to get to good core code and documentation that meets both Drupal's coding and documentation standards. Whether it happens in this issue or in additional ones does not matter. As you might note, I have opened up a number of follow-up issues to improve the quality of the Coder reports and have several more on my personal 'To Do list' to open as well.

@Lars - I fully agree with your thoughts. I think we can focus on good core code and documentation in other separate issues. Otherwise it will become quite difficult to complete one dedicated task.

I am not sure how should I incorporate a patch for breakpoint_enable() as it needs to be put into completely new file breakpoint.install. Patch seems to be work with file modifications but not with the new additions. I don't know what to do here.

I opened #1820512: Move Breakpoint module closer to meeting Drupal standards so that we can correct the "extraneous" review items from #7 that are not corrected in this issue.

My concern is not whether the items get corrected in this issue or that that one. My goal is simply if something is wrong, that item needs to be corrected!

@amitgoyal I'll reroll the patch to include the new file. Can you tell me how you generated the report (coder module or drupalcs)?

I checked some other core modules, hook_install, hook_uninstall, hook_enable and hook_disable are defined in module.install so I'll move all of them and add the necessary @see's

Do you want other things fixed as well?

@amitgoyal re: #16 -- I must admit my git skills are still in the early stages of development. However, I recently learned that one can add a new file through a patch.

The way to to do this is as follows (please allow for simplification):

1) While in the git development/change branch, add a new file with the appropriate code and save it.
2) Use the "git add "new-file"' command to add the new file for saving in that branch.
3) Commit all of the edit changes as usual (including the new file).
4) Generate a new patch in usual manner.

The resulting patch should include the new file you added as well as any other edits to existing files. Before posting the patch to drupal.org, as a rule, I open up the patch to visually check that the expected changes have been included.

I would happily roll a patch in this issue myself. However, if I create a patch in this issue, my understanding of the Drupal core process is that neither you nor I could review the result and promote it to RTBC status. Hence, I think it is best that I do not roll anything in this issue so that I can maintain my pure "reviewer" role.

Sorry @attks - I did not mean to unassign you with that last comment.

I would appreciate if you could could include fixes for as many of the items identified in #7 here. That would make whatever else is left for #1820512: Move Breakpoint module closer to meeting Drupal standards smaller.

@attks - I think you can work on one of the issues mentioned here #1518116: [meta] Make Core pass Coder Review.

@Lars - I make require changes in files and use command 'git diff > abc.patch' to create the patch. Afterwards, I revert my changes in those files using command 'git checkout file' and then test the patch by applying it using command 'git apply -v abc.patch'.

Please clarfiy in more detail regarding "4) Generate a new patch in usual manner.". And then how will I rollback my changes so that I can test my patch.

@amitgoyal are you going to reroll or do you want me to do it?

@attks - I will reroll.

Here is the updated patch which will move hook_enable() from breakpoint.module file to breakpoint.install file.

Looks good to me, assuming testbot will be happy as well

there, new component added.

Thank you @jhodgdon!

Tum te tum...

Did someone actually test this update? I'm concerned that since a function was moved from the module file to the install file, things may have changed, and I'm not sure the testbot is enough of a test, since it must have passed testbot without this patch too... ??? Anyway it makes me nervous and I'm going to defer to someone else to commit it unless someone can confirm they tried enabling the module and didn't get any errors, especially in the case where some other module actually defined breakpoints, from the UI.

Also, could someone correct this spelling error in breakpoint.php:

-                // We need to allow vendor prefixed media fetures and make sure we
-                // are future proof, so only check allowed characters.
+                // We need to allow vendor prefixed media fetures and make sure
+                // we are future proof, so only check allowed characters.

"fetures"... is that supposed to be "features"? I don't even know for sure, since none of this comment actually makes any sense to me. If you feel the spell correction is out of scope, please file a separate issue...

#30 I'm sorry that #26 wasn't clear. I installed from scratch, enabled breakpoint module and checked the config, breakpoints from the themes are imported.

New patch to fix the typo.

Pretty please use real tables. :)

Added follow up issue 1820868.

Added a folloow-up issue 1821592.

Added follow-up issue 1821616.

I added a patch to #1820512: Move Breakpoint module closer to meeting Drupal standards to cover the rest of #7 review items that were not included in the patch in #31. Reviews there would be welcome.

Thanks attiks! The previous patch was RTBC and this one is the same except the spelling fix, and I'm convinced... I'll get it committed soon.

Thanks all! This is committed to 8.x.

#12: "Also, please let me know if you have any other code patches that you would like me to review like #7 before they get committed to core."

If you have time, feel free to review #1775530-24: Move picture into core