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 |
---|---|---|---|
#42 | dashboard-1332658-d7-42.patch | 8.75 KB | Albert Volkman |
#39 | dashboard-1332658-d7-39.patch | 9.22 KB | Albert Volkman |
#36 | dashboard-1332658-36.patch | 9.4 KB | xjm |
#36 | interdiff.txt | 1.27 KB | xjm |
#34 | dashboard-1332658-34.patch | 9.55 KB | David_Rothstein |
Comments
Comment #1
sven.lauer CreditAttribution: sven.lauer commentedAnd another quick one.
Comment #2
jhodgdonLooks pretty good, thanks! A couple of things need fixing:
a)
expand -> expands
b)
Display dashboard -> Displays the dashboard.
c)
Leave a blank line before the @see.
d) dashboard_admin_blocks() needs @see for dashboard_menu(), and needs the line saying what path(s) it pertains to.
Comment #3
jhodgdonAlso, looking through the dashboard.module file, there are a couple of other functions that haven't been fixed up and should be, such as:
Comment #4
sven.lauer CreditAttribution: sven.lauer commentedRe-rolling. With respect to #3: I thought that it would still be useful to retain the information that the function is intended as an ajax callback, so I went with "Ajax callback: VERBs ..." --- though, technically, I guess those are just menu callbacks.
Comment #5
sven.lauer CreditAttribution: sven.lauer commentedComment #6
jhodgdonWhat you did for the Ajax callbacks is probably fine. But if they are hook_menu() callbacks, they should also have the path?
Also, this needs a newline after the file docblock (sorry, probably didn't catch that last review):
+/**
+ * @file
+ * Ataches behaviors for dashboard module.
+ */
(function ($) {
And while we're at it, this could use "the":
+ * Adapts block's position to stay connected with mouse pointer.
Actually should probably be:
Adapts a block's position to stay connected with the mouse pointer.
And this could use "the":
+ * Page callback: Displays dashboard.
Comment #7
sven.lauer CreditAttribution: sven.lauer commentedGood point about the path's for the ajax callbacks.
Also added the "the"s and the blank line.
Comment #8
xjmThanks @sven.lauer! I noticed a few small things about the patch:
Some trailing whitespace here.
It might be good to make this a little more clear, maybe, "Sets up the drag-and-drop behavior and the 'close' button."
These two are kinda comma splices. I'd say either make it two sentences, or (better) simply omit the comma and the word "used."
After applying the patch, I also found a few more things in the dashboard module that we can probably fix:
DashboardBlocksTestCase
does not have a docblock, and its test cases need the right verb tense in their summaries. Some are also two lines.Drupal.behaviors.dashboard
could probably have it summary changed to "Implements..." rather than "Implementation of..."dashboard.api.php
, should the hook summary begin "Add" rather than "Adds"? Reference: http://drupal.org/node/1354#hooksThanks for your work on all these!
Comment #9
sven.lauer CreditAttribution: sven.lauer commentedFixing all of these except one (and yes, you are right, hook documentations should be in imperative form, so "add" instead of "adds"):
What is sometimes called "comma splices" is perfectly grammatical in English. So much so that even Lynn Truss, who is the source and/or progenitor of many LIES about English grammar and orthography, says: "so many highly respected writers observe the splice comma that a rather unfair rule emerges on this one: only do it if you're famous." (By "observe" she seems to mean "use".)
Of course, a managing editor (== doc lead) may decide that random rules are enforced, but unless I hear from one of them, I refuse to abide by those.
Note that the change you are proposing "Returns HTML for disabled blocks, used in dashboard customization mode." => "Returns HTML for disabled blocks in dashboard customization mode." would actually change the meaning, if subtly.
Comment #10
sven.lauer CreditAttribution: sven.lauer commentedUhm, sorry.
I did not mean to be disrespectful. I appreciate your reviews.
As a linguist by trade, I just have this visceral reaction to invocations of pseudo-rules of English. And there is a lot of misinformation out there, so it is perfectly understandable that someone would want to enforce these pseudo-rules (see also "split infinitives" and "stranded prepositions"). In my profession, we don't believe in these pseudo-rules (given that many well-respected prose writers use the allegedly unacceptable forms, and English (luckily!) does not have an Academie to regulate what is right).
So, please forgive me if I react strongly against certain prescriptions: It's just what I've been taught to loathe.
Comment #11
xjmWell, documentation standards are, by their nature, prescriptive, as is any formalized writing really. Different registers. :) I'd use constructions here in a comment that I would never use in the API documentation. Contractions, for instance. Incomplete sentences.
Edit: Also, as a note. Perhaps "comma splice" is just what the 8th grade English teachers call what is (to me) a comma that "sounds wrong." Those commas above sound wrong. That's not how a comma sounds.
Edit 2: I wonder if you would have had a less visceral reaction if I'd not called it a "comma splice"? Hmmm. Of course, none of this is on topic, but perhaps we can debate it some time. Nonsense up with which I will not put! ;)
Comment #12
xjmHmm, reading this:
I guess that's part of my discomfort with these two summaries. "Used" is ambiguous. Is the HTML used? The disabled blocks? The function itself? The comma does not denote any sort of relationship between "used" and a specific subject. I don't know what the intended meaning is.
However, we both agree it's a minor and (clearly) debatable point, so #9 is probably RTBC pending @jhodgdon's OK. ;)
Comment #13
jhodgdonRegarding comma splices and other usage/grammar/punctuation issues, on drupal.org we have some editorial standards for documentation, and we generally try to follow them in our API docs too.
http://drupal.org/style-guide/content
Basically, we use standard style references for stuff that isn't Drupal-specific. My style reference mentions comma splices as not being standard American English good style.
Anyway, whether or not we care about comma splices, I agree that the comma is not the best punctuation in those two examples mentioned in #8 item 3. I think we can do better at writing this documentation. For instance:
How about:
Returns HTML for disabled blocks, for use in dashboard customization mode.
Comment #14
sven.lauer CreditAttribution: sven.lauer commentedRe-rolled, adopting the wording suggested by @jhodgdon in #13.
And I do agree that standards are prescriptive (that is why they are standards), which is also why I said that, in the end (in absence of a written standard regulating this), jhhodgdon has final word. And I submit that "comma splices" can be acceptable in quite formal prose (which is not to say that I advocate randomly squishing together independent sentences). Anyhow, the issue is settled.
Comment #15
sven.lauer CreditAttribution: sven.lauer commentedComment #16
xjmI think dashboard-rtl.css is still missing its
@file
block. Outside of that, this patch looks ready to me.Comment #17
sven.lauer CreditAttribution: sven.lauer commentedRight. Also, dashboard.module actually still did not have a
@file
block.Fixing both in the attached patch.
Comment #18
xjmAh, missed that too. I don't see the
@file
fordashboard.module
in that patch, though?Comment #19
sven.lauer CreditAttribution: sven.lauer commentedThat is ... odd. I put it there, and it is in my local branch ... but not in the patch.
Anyhow, here is the patch with the @file header.
Comment #20
xjmAlrighty, everything looks correct now. Thanks @sven.lauer!
Comment #21
David_Rothstein CreditAttribution: David_Rothstein commentedDo we really not want a blank line there (and other similar places in the patch)? Pretty much all of core has the blank line.
It makes more sense to me with one, since we always put blank lines before docblocks everywhere else, to separate them from what came before.
Comment #22
jhodgdonYou mean at the very top of the file? Agreed, it looks like a lot of core has a blank line, but I don't have a strong opinion on whether there should or shouldn't be one. Does it add a lot to readability to have one there?
Comment #23
xjmI think literally almost all of core has the blank line. In my patches I was removing a second line where there were two, but leaving when it was one. I guess I think it looks a little nicer with the blank line, but mostly it's probably best to be consistent.
Comment #24
jhodgdonI bow to your greater wisdom. :)
Comment #25
xjmPffffff. :P
Here's a reroll of @sven.lauer's patch that simply restores the blank line in PHP files.
Comment #26
jhodgdonokeydokey!
Comment #27
David_Rothstein CreditAttribution: David_Rothstein commentedSorry, just noticed this ("adminstrative" => "administrative"). I'd reroll but then I'd get commit credit for a patch I didn't actually do anything meaningful for :)
Comment #28
xjmWell, I know webchick at least prunes rerollers and other random-attachment-adders from the commit mentions. Dries, however, just uses whatever dreditor says. Not sure about catch. ;)
Since I already rerolled once, I'll take the risk!
Comment #30
xjmRight, yeah, that second isn't a patch. ;)
Comment #31
David_Rothstein CreditAttribution: David_Rothstein commentedI need to stop proofreading. (See the attached interdiff.) Now I too can be added to the commit list, I guess...
With this little fix I think it's good to go.
Comment #32
David_Rothstein CreditAttribution: David_Rothstein commentedActually, "administrator-only" doesn't really make sense. There is nothing about those blocks that make them only for administrators.
How about "administrator-flagged" instead?
Now I can say I've actually contributed to this patch... sort of :)
Comment #33
xjmMaybe just "administrative"?
Comment #34
David_Rothstein CreditAttribution: David_Rothstein commentedDefinitely seems reasonable. Since that's the only change I'm putting this back to RTBC.
Comment #35
David_Rothstein CreditAttribution: David_Rothstein commented#34: dashboard-1332658-34.patch queued for re-testing.
Comment #36
xjmStripping "Paths: " lines per #1315992: No standard for documenting menu callbacks.
Comment #37
Dries CreditAttribution: Dries commentedCommitted to 8.x. Moving to 7.x.
Comment #38
xjmI checked #36 for the "new" standards, and while there are several changes to summaries that add "Foo callback:" prefixes, in all cases it's just a reordering of words already there that makes the summaries follow our general standards.
Comment #39
Albert Volkman CreditAttribution: Albert Volkman commentedD7 backport.
Comment #40
Albert Volkman CreditAttribution: Albert Volkman commentedComment #41
xjmThanks @Albert Volkman! Everything there looks to be okay to backport, except for these parts:
We need to not add these
@see
to the menu hook implementation in D7. (As I stated above, though, I think the changes to the summaries of those same callbacks are fine because they are really just reordering words that are already there.)Comment #42
Albert Volkman CreditAttribution: Albert Volkman commented@see dashboard_menu() removed!
Comment #43
Albert Volkman CreditAttribution: Albert Volkman commentedComment #44
xjmLooks good to go now; thanks!
Comment #45
webchickAll of this makes sense except for:
I thought we always put S on the end of the first word of the function descriptor.
Comment #46
David_Rothstein CreditAttribution: David_Rothstein commentedI thought hook documentation does it the other way, without the "s" (although I'm not entirely sure why).
Comment #47
jhodgdon#46 is correct. Hook documentation is not the same as others, because we're documenting "If you want your module to do X, implement this hook", not "This function does this action".
Comment #48
webchickOh bloody hell. :)
Ok, committed and pushed to 7.x. Thanks.