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.
Part of meta-issue #1310084: [meta] API documentation cleanup sprint.
Comment | File | Size | Author |
---|---|---|---|
#37 | comment_docs-1332598-37.patch | 16.35 KB | Albert Volkman |
#37 | interdiff.txt | 10.19 KB | Albert Volkman |
#35 | comment_docs-1332598-35.patch | 20.2 KB | Albert Volkman |
#35 | interdiff.txt | 1.01 KB | Albert Volkman |
#33 | comment_docs-1332598-33.patch | 20.79 KB | Albert Volkman |
Comments
Comment #1
sven.lauer CreditAttribution: sven.lauer commentedComment #2
jhodgdonThis is mostly good, thanks! A few things to fix:
a)
Needs a blank line after the @file docblock.
b)
I think this should be @see comment_menu() ?
c)
submit -> submission
d)
--> for the conformation form... (needs "the"). Also:
and possibly other similar lines.
e)
If this is a form constructor, it can't have a true/false return value?
f)
Original was better here. Refer to modules as "the Comment module" or "comment.module" (without "the").
g)
--> Respond to updates...
h)
I wasn't sure here if you removed the newline by mistake, or if the original had no newline?
i)
Probably should follow http://drupal.org/node/1354#menu-callback format? Such as:
Menu loader callback: Loads the ...
j)
Second line needs one more space of indentation.
k)
Needs reformatting (newline after @return). There's at least one farther down too:
l)
- First line: Display -> Displays
- Last line: @params -> @param
m)
Needs "the". Same for the next few docblock starting lines. Also this group:
(and following docblocks). Actually... "Sets comment subject setting." ??!? That makes no sense to me. How about just "Sets the comment subject."?
Comment #3
sven.lauer CreditAttribution: sven.lauer commentedAttached patch addresses these issues. Thank you for your patience ;)
With respect to m), you suggestion "Sets the comment subject." would not work, because the function does not actually set a subject, but rather sets a value for the "subject setting" (i.e. the setting specifying whether a subject field should be displayed on comment forms).
I tried to make this clearer and settled on the format "Sets setting: ", in order to avoid the awkward "Sets the 'xyz' setting" formulation. I could re-roll if you have a better suggestion for those.
Comment #4
jhodgdonHm.
I am not fond of this formatting, especially the question mark at the end, and the "sets setting" part...
How about something like this:
Sets a value telling whether the comment form should have a subject field.
or
Sets the value governing whether comment forms have subject fields.
or something like that?
Comment #5
sven.lauer CreditAttribution: sven.lauer commentedI did change all these to description along the lines of your suggestion.
I also fixed a number of @file headers which referred to "comment module" etc. instead of "the Comment module".
Comment #6
jhodgdonThis is looking pretty good! A couple of things to fix still:
a) Extra blank line here:
b) I don't think we want @ingroup themeable for theme preprocess functions?
http://drupal.org/node/1354#themeable
c)
Either comment.module or the Comment module (no dot in the middle).
d) should fix formatting for the @params here as well as the @return that you fixed:
e) Needs to end in .:
This also needs . at end:
Comment #7
sven.lauer CreditAttribution: sven.lauer commentedFixing the issues mentioned in #6.
Comment #8
jhodgdonOK. There are still a few issues here (sorry if I missed them the last review):
a) (header/handler):
+ * Form validation header for comment_admin_overview().
b) This line needs some sentence rearranging:
+ * Form constructor the for confirmation form for comment deletion.
c) And if you have to re-roll anyway, you could fix this:
+ * An object representing info on the node type. The only property that is
Should say "An object representing the content type."
d) This should also be fixed. Use "ID" not "cid" or "id" in text, to refer to an identifier:
Here's another one:
* Node-id to count comments for.
e) This needs a "the":
+ * Page callback: Publishes specified comment.
f) Lists generally need a header before them:
So before 0: there should be a line saying something like "The level of contact information allowed:"
g) There are still a few places that say "comment module" instead of "Comment module" or "comment.module", such as:
+ * Tests that comment module works when enabled after a content module.
+ * Tests that comment module works correctly with plain text format.
Comment #9
sven.lauer CreditAttribution: sven.lauer commentedFixing those.
With respect to (d) and (g): I grepped through the module directory to make sure I have not missed any of those. Found some (d) and a lot of (g) additional instances, mostly in lines I had not touched so far.
Comment #10
xjmAlright, I read through the patch but did not apply it. Some of what follows are suggestions to change wordings that existed before the patch, so we can fix them here or submit followups, whichever is better.
My first thought on reading this is "Attaches what behaviors?" (for comments, or the Comment module, etc.)
Presumably this confirmation form is the result of clicking a button on another form. Can we add an @see to the parent form constructor?
Looks like this form doesn't have its own menu path but is instead used in
comment_confirm_delete_page()
, so let's add @see linking these two functions.This is sort of a trivial point, but do we need to specify Drupal here?
The string "menu loader callback" doesn't seem to be used anywhere else in core that I could find, nor anything close to it. This is the bit of the
hook_menu()
doc that explains wildcard autoloaders:Can we check and see how other callbacks like this are documented? (Edit: If they're documented at all. Maybe we need to be trend-setters.)
Semantic nitpick: the comments are being displayed, not their IDs.
I know this text exists before the patch, but what does "noting indentation level" mean here? Maybe we could make the summary a little clearer.
Should there be a comma after "delete" here?
Maybe just "Constructs a render array," possibly with an @see to drupal_render()? (That's the form used elsewhere in the patch for render arrays.)
We can probably add some articles here.
Should we also improve this text while we're reformatting it? Presumably "the result" is the number of comments on the node. And what might cause an error?
This is the form submission handler for the Preview button for some form, correct? If so, let's use the Form API-style docblock.
Maybe we could add the deleted information from the old summary in a second paragraph? Not sure if this is necessary or not.
Should these say "preprocesses" rather than "processes"?
These should probably be "a sorting code." (I apologize because I think I wrote these docblocks...)
This parameter is optional, correct? Should it begin "(optional)" and end with "Defaults to NULL"?
Checks what for contact info? (Again, not sure whether we need to fix this in this patch or not.)
Through the node interface?
I think it would just be "clears watchdog"? ("Clears the watchdog" makes me think we're referring to an actual dog. The furry kind.)
Note that there are two issues open that involve significant changes to the
commentComment module, so some patches may need to be rerolled if others go in first:Comment #11
sven.lauer CreditAttribution: sven.lauer commentedFixing all of these, except:
13) I chose to not add the information back in ... what the submission handler does is pretty much the usual thing, so I don't think there is any reason to note this especially.
Comment #12
jhodgdonMuch better! There are still a couple of minor things that I probably missed last time (sorry!)... If you make a new patch, posting an "interdiff" would be helpful to aid in reviewing, if you have the tools to make one. Ask in IRC for help if not. :)
a)
This needs an extra space of indentation:
b)
Wrong verb tense.
c) List formatting:
The line before the list should end in :
d) Needs an apostrophe (commenter's):
e)
Huh? Sets a comment setting -- what does that mean? And maybe needs "a" or "the" before article content type?
Comment #13
sven.lauer CreditAttribution: sven.lauer commentedHere is another patch with an interdiff ... I actually tried to do one for the last patch, as well, but that proved difficult, as I had pulled in upstream changes in the meantime that created conflicts.
Re: e) This is what the function does---it set a setting for use in a test case. I changed this to "Sets a comment setting variable ...", but I am not sure this is much better ...
Comment #14
jhodgdonLooks good -- thanks!
Comment #15
catchSorry folks, no longer applies so will need a re-roll.
Comment #16
sven.lauer CreditAttribution: sven.lauer commentedRe-roll attached. Setting back to RTBC because all I did was fixing the merge conflicts.
Comment #18
jhodgdon#16: doc-cleanup-comment-module-1332598-9.patch queued for re-testing.
Comment #19
jhodgdonComment #20
catchCommitted/pushed to 8.x, thanks!
Comment #21
jhodgdonbackport time!
Comment #22
oriol_e9gDocs improvements backported except the sorting functions because we still use vancodes in Drupal 7.
Comment #23
xjmThanks @oriol_e9g! Few things:
The menu callback changes aren't getting backported to D7, so we should remove or reduce this and similar hunks to fixes for the standards that existed previously.
Trailing whitespace here. Hopefully that wasn't in the original patch. :P
Comment #24
oriol_e9g@xjm Wow! You are in all issues queue!!!
Comment #25
jhodgdonI don't think we're applying the new menu callback standards to Drupal 7 patches?
Comment #26
xjmRight, like I said in #23. :) The
Path: blah blah
and@see foo_menu()
need to be removed from #24.Comment #27
oriol_e9gWhich is the coding standard for @see in Drupal 7? :/
Comment #28
xjmRemove this line too. It's part of the menu callback standard added in #1315992: No standard for documenting menu callbacks, and so it is not backportable to Drupal 7. Thanks!
Comment #29
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedUpdated patch with interdiff from #27.
Comment #30
jhodgdonSorry for the delay! Finally getting around to reviewing some of these issues... There are a couple of problems I can see in this patch:
a) comment.admin.inc near the top:
present -> presents
b) hook_comment_view_alter() doc in comment.api.php:
There is no hook_preprocess_comment(). This should say "implement hook_preprocess_HOOK() for comment.tpl.php".
c)
The class here needs a doc block.
d) also in comment.test
The proper type should be "bool" not "Boolean" or "boolean" in @param/@return. See http://drupal.org/node/1354#param-return-data-type -- and this applies elsewhere in this file too.
e)
$approval is misspelled in the @param.
f)
Refer to the module as "the Comment module". Several other spots in comment.test too...
Comment #31
Lars Toomre CreditAttribution: Lars Toomre commentedThere is a code problem in comment.module where 'use Drupal\comment\Comment;' occurs after the declaration of various constants.
Comment #32
Lars Toomre CreditAttribution: Lars Toomre commentedHere is a work in progress patch after doing a full review of the files in the Comment module. This patch still includes numerous ??? indicators for missing text substitutions.
Comment #33
Albert Volkman CreditAttribution: Albert Volkman commentedRe-roll of #32. Filled out ???'s, removed extraneous changes. I'm sure this will still need some work. Setting back to 8.x.
Comment #35
Albert Volkman CreditAttribution: Albert Volkman commentedSome code from another patch sneaked in. Also fixed a whitespace error.
Comment #36
jhodgdonThanks for the patch, and sorry for the review delay...
So, the changes in this patch for comment.api.php are all wrong (they incorrectly changed the verb tense for hook definition documentation). Just revert those.
The other changes mostly look good. A few minor corrections:
a) Punctuation of this change in comment.module -- : vs. ;
b) later in comment.module -- this @throws should be at the end of the documentation, not right after the @param:
c) Also in comment.module:
That "Defaults to NULL" is confusing and unnecessary, given that the previous sentence explains exactly what happens if no language is provided.
d) Actually, take out all those "Defaults to ..." statements that just say the value that is already in the function signature. Our standards do not recommend doing that:
http://drupal.org/node/1354#param
e) We also need to be careful about things like this (from comment.module again):
This "Required if $comment is not given" goes with $context presumably, not with 'cid' (which shouldn't be in quotes anyway). So it shouldn't be part of the list item.
f) in CommentTestBase.php
$approval is misspelled in the docs.
g) And a bit lower in CommentTestBase.php:
Comment should not be capitalized here.
Comment #37
Albert Volkman CreditAttribution: Albert Volkman commentedNo reason to apologize at all. You're a busy gal :) I appreciate your hard work.
Comment #38
Albert Volkman CreditAttribution: Albert Volkman commentedComment #41
no_angel CreditAttribution: no_angel as a volunteer commentedComment #42
no_angel CreditAttribution: no_angel as a volunteer commentedManually test comment_docs-1332598-37.patch - failed.
Needs reroll.
Comment #43
jhodgdonThis patch is 3 years old. I think at this point we can close the issue as "won't fix" and start over with anything left. Besides which, for coding standards fixes we are generally not trying to fix them by module, but by type of fix.