Problem/Motivation

Several docblocks are out of coding standard.

Proposed resolution

Document @return type and @param values, add descriptions for them.

Remaining tasks

The patch in #9 was committed. Changes from the patch in #1 were not included in that patch and thus not committed.
Next step is: take the patch from #1 and fix/add the suggestions from #2.

CommentFileSizeAuthor
#13 feeds-docblock-2490782-13.patch13.78 KBmegachriz
#12 feeds-docblock-2490782-12.patch14.34 KBmegachriz
#9 interdiff-2490782-6-9.txt4.94 KBmegachriz
#9 feeds-docblock-2490782-9.patch6.18 KBmegachriz
#6 2607300.patch5.04 KBheykarthikwithu
#1 feeds-7.x-2.x-2490782-1-docblock.patch26.4 KBdonquixote

Comments

donquixote’s picture

donquixote commented
Status: Active » Needs review
StatusFileSize
new feeds-7.x-2.x-2490782-1-docblock.patch26.4 KB

I am sure a lot of this could be further improved.
But I think it is a step forward.

megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Needs review » Needs work
Related issues: +#2342143: Fix coding standards

Related issue (not a duplicate): #2342143: Fix coding standards

I think that descriptions for each @param should be added. For hook implementations there should be no documentation for the parameters and the return value. This should already be documented in the hook documentation. See also "Hook implementation" at https://www.drupal.org/node/1354

donquixote’s picture

donquixote commented

I think that descriptions for each @param should be added. For hook implementations there should be no documentation for the parameters and the return value.

While I agree, I think this is ok to postpone to a follow-up issue. Making the IDE happy, and documenting the type, is already a huge win imo.

For hook implementations there should be no documentation for the parameters and the return value.

That's a fair point.
For my own modules I still want the @param and @return on these functions, simply because it is nice for my IDE. But I understand why the general consensus / standard for drupal says otherwise.

megachriz’s picture

megachriz
Primary language Dutch
commented

Closed #2607300: Add @return type and change @param values in the comment docblocks. as a duplicate which has a patch with more nice docblock improvements.

heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
commented
Assigned: Unassigned » heykarthikwithu

Working on this.

heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
commented
Assigned: heykarthikwithu » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 2607300.patch5.04 KB
megachriz’s picture

megachriz
Primary language Dutch
commented

@heykarthikwithu
Can it be that you uploaded the wrong patch? It is the same as in #2607300: Add @return type and change @param values in the comment docblocks..

heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
commented

@MegaChriz, yes uploaded patch is the same in #2607300: Add @return type and change @param values in the comment docblocks.

megachriz’s picture

megachriz
Primary language Dutch
commented
StatusFileSize
new feeds-docblock-2490782-9.patch6.18 KB
new interdiff-2490782-6-9.txt4.94 KB 
  1. +++ b/feeds.api.php
@@ -405,10 +405,6 @@ function my_module_summary_callback(array $mapping, $target, array $form, array
- * @return array
- *   The per mapping configuration form. Once the form is saved, $mapping will
- *   be populated with the form values.
- *

    This should be kept. 

  2. +++ b/libraries/common_syndication_parser.inc
@@ -13,9 +13,10 @@
+ * @return array|bool

@@ -43,7 +44,8 @@ function common_syndication_parser_parse($string) {
+ * @return string|bool

@@ -478,9 +480,10 @@ function _parser_common_syndication_RSS20_parse($feed_XML) {
+ * @return bool|int

@@ -516,7 +519,8 @@ function _parser_common_syndication_parse_date($date_str) {
+ * @return bool|int

+++ b/plugins/FeedsCSVParser.inc
@@ -56,7 +56,7 @@ class FeedsCSVParser extends FeedsParser {
+   * @return mixed|bool

    If the only boolean that is returned is 'false', then it is better to document @return false instead of @return bool (at least that is how I've seen it in core, see \Drupal\Component\Utility::strpos()).

Made corrections in the attached patch. Note: changes from the patch in #1 not included.

  • MegaChriz committed ea2c278 on 7.x-2.x 
    Issue #2490782 by MegaChriz, heykarthikwithu: fixed a few comment...
megachriz’s picture

megachriz
Primary language Dutch
commented
Issue summary: View changes
Priority: Normal » Minor
Status: Needs review » Needs work

Committed #9.

The improvements from #1 are not included. That patch still needs work (see #2).

megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Needs work » Needs review
StatusFileSize
new feeds-docblock-2490782-12.patch14.34 KB

This is a partly reroll of the patch in #1. I left a few things out that were either covered by #2342143: Fix coding standards or by other issues.

megachriz’s picture

megachriz
Primary language Dutch
commented
StatusFileSize
new feeds-docblock-2490782-13.patch13.78 KB

Reroll + small modifications.

  • MegaChriz committed 9489d88 on 7.x-2.x authored by donquixote 
    Issue #2490782 by MegaChriz, donquixote: Added parameter types for a few...
megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Needs review » Fixed

Committed #13.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.