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 |
---|---|---|---|
#40 | drupal-1357138-40.patch | 33.79 KB | tim.plunkett |
#38 | forum-clean-up-documentation-1357138-38.patch | 35.03 KB | jstoller |
#33 | forum-clean-up-documentation-1357138-33.patch | 34.37 KB | jstoller |
#33 | interdiff.txt | 9.46 KB | jstoller |
#31 | interdiff.txt | 3.11 KB | jstoller |
Comments
Comment #1
xenophyle CreditAttribution: xenophyle commentedComment #2
jhodgdonLooks pretty good! A few things to fix:
a)
First line of @file docblock should be a one-line summary. Not your fault, but the wording is bad here too (indicated -> indicating). I think we have standards for .tpl.php file docs anyway? Yes:
http://drupal.org/node/1354#templates
Note: This applies to forums.tpl.php at the end of the patch as well as forum-submitted.tpl.php near the beginning, and may also need to be fixed in the other forum module tpls as well?
b)
Maybe "forum containers"?
c)
typo -- constutor -> constructor
d)
In other cleanup patches, we're either calling these "Pre-render callback:" or "Render API callback:" See Sven's patches at #1347890: Clean up API docs for file module and the discussion about standard or not standard on that issue).
Also, callback doc headers like this don't really need @return sections.
e)
Typo I think: form -> forum
f)
Why is this "nodes are new" sentence relevant to this function? Maybe instead describe what "active" means (is it the most "new" posts?) in the $sortby parameter?
g)
Needs . at end
h)
I don't think these should have $ in front of them? And they aren't really "arguments", maybe "elements"?
(same applies to next several functions)
i)
Maybe this should either explain what the code system is, or refer to another function that documents it?
Comment #3
xenophyle CreditAttribution: xenophyle commenteda) I wasn't sure if file headers had to be limited to 80 characters. I would like to update to documentation to say so, but I am not able to edit that page.
h) I also thought this was weird, but it seems that all the other modules do it this way. Shall we make an official standard?
Comment #4
jhodgdona) It's in the "general" section at the top -- the 80-character limit applies to all summary lines. But I'll add it to the File section too.
h) I don't see why we need a new standard? We only use $ for parameters and variables in PHP. These are not function arguments/parameters. They are array elements. They cannot be referred to with $ in the PHP code, so they also shouldn't be referred to with $ in the docs?
Comment #5
xenophyle CreditAttribution: xenophyle commentedComment #6
jhodgdonSorry, didn't notice this before. The .tpl.php files should follow:
http://drupal.org/node/1354#templates
Comment #7
xenophyle CreditAttribution: xenophyle commentedFor the .tpl files, I modified the first line under @file and moved the "subvariable" documentation up.
Comment #8
sven.lauer CreditAttribution: sven.lauer commentedThis is looking pretty good. Here are a few things I noticed:
1) css files lack @file header.
2) The doc headers of most methods in forum.test do not follow standards (function standards apply).
3) This function doc does not follow standards:
4) Some preprocess functions are documented with "Processes ...", while other with "Preprocesses ..." --- best be consistent?
Comment #9
sven.lauer CreditAttribution: sven.lauer commentedComment #10
xenophyle CreditAttribution: xenophyle commentedNew patch with the requested changes.
Comment #11
jhodgdonWe had a discussion on #1315992: No standard for documenting menu callbacks and we have (unfortunately) changed the standard for hook_menu() callbacks:
http://drupal.org/node/1354#menu-callback
So this patch will need an update (no paths and they need @return sections).
Comment #12
jhodgdonTagging.
Also @xenophyle: are you still planning to work on this? If so, please respond. If not, we'll unassign it so someone else can. Thanks!
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedSince there hasn't been activity for 4+ weeks, I'm going to move this back to unassigned.
Comment #14
jstollerUpdated patch from #10 with new hook_menu() callback documentation standards.
Comment #15
jstollerMissed a couple white space characters.
Comment #16
hosef CreditAttribution: hosef commentedThere are two places where there is unnecessary white-space in lines 196 and 649, and '@ingroup themeable' is also being added to the tpl.php files under issue http://drupal.org/node/716496 , but other than that, it looks ok.
Comment #17
hosef CreditAttribution: hosef commentedIgnore my previous comment this looks like it is ready to go.
Comment #18
jhodgdonIt looks pretty good, thanks! But it's not quite ready to be committed.
a) forum.admin.inc
Last line there needs to end in .
b) I took a look at forum.module, and this also needs to be fixed (around line 603):
See http://drupal.org/node/1354#hookimpl for standards (2nd example)
c) Also in forum.module:
There is no such hook. This is really an example of:
http://drupal.org/node/1354#hookimpl (example 3)
d) The *.css files do not have @file blocks, and should.
Comment #19
jstollerNew patch with requested changes.
Comment #20
jhodgdonDang. We are over thresholds for major/critical issues, and this patch will conflict with the fix for
#787652: Forum vocabulary alterations possibly obsolete -- possible to delete forum vocab
So I can't commit it now, and once that gets in (should be in a day or two), I expect this patch will need a reroll.
The patch looks pretty good though! The only things I think might need to be changed:
a)
- I think RTL should be written out as right-to-left.
- "the forum module" --> Forum should be capitalized when it's the name of the module. It would be good to check this in the other @file headers for the other files too. [I specifically noticed that forum.css has the same problem; probably others too.]
b) In forum.module:
There is no theme_block(). It should be block.tpl.php.
c) in forum.module:
- These two should use the same format -- I think the second should say "Preprocesses varialbes for whatever.tpl.php"
- The next several functions in the file have the same problem.
d) in forum.test:
We actually are not putting doc blocks on test methods setUp(), tearDown(), or getInfo().
Comment #21
jstollerNew patch with changes requested in #20.
Comment #22
jhodgdon#21: forum-clean-up-documentation-1357138-21.patch queued for re-testing.
Comment #23
jhodgdonThat other patch got committed so I requested a retest -- probably this needs a reroll.
Comment #24
jstollerIt looks like that other patch was rolled back and this one still applies cleanly. Any chance we could slip this one in?
Comment #25
jhodgdonNo, we can't slip this one in. That one is on the critical/major list. This one isn't. We are over thresholds for criticals/majors. Sorry.
Comment #26
jhodgdonThis is getting very close! Looking at the patch in #21, I found a few more things to fix:
a) forum-list.tpl.php [I wouldn't have even mentioned this except there were other things, so might as well get this right too]:
Normally we would word this:
"TRUE if ...; FALSE if ..."
rather than "Is TRUE if.... etc."
b) forum-list.tpl.php
True -> TRUE
c) forum-topic-list.tpl.php
Is "outputtable" really a word, and what does it mean here? Maybe this could be reworded to say "A string representing ... . Safe to output." like in the other list items?
d)
Needs comma before "and".
e) in forum.module
Normally we would leave out "returns" inside @return docs.
f) in forum.test
Why was this indented?
Comment #27
xjmRegarding f, the standard says
@todo
are supposed to be indented: http://drupal.org/node/1354#todoComment #28
jhodgdonduh. Thanks xjm for reading the standards. Wonder when that went in... I don't remember it but it makes some sense. :)
Comment #29
jstollerNew patch with changes requested in #26.
Comment #30
jhodgdonCould we get an interdiff?
Comment #31
jstollerHere's an interdiff between #21 and #29.
Comment #32
xjmThanks @jstoller. I confirmed that the interdiff in #31 covers all the points in #26.
I reviewed the latest pacth in #29 and found these things:
I'd use a semicolon here, but I'm not sure that this isn't just a personal preference on my part.
I think this parameter is optional, so we should document it as such.
I think there should be a comma after "individual comments".
This is out of the scope of the patch, but, wow, magic numbers. Can we open a followup issue to replace these with constants?
We should change this to use standard list formatting and add descriptions for the parameters, correct?
This seems to be missing the parameter description? Also, are there other keys?
Can we specify here whether it is HTML or a render array?
This docblock seems to be entirely wrong to me. Menu administration?
How about simply "Provides automated tests for the Forum module."?
"Logged-in" should be hyphenated.
This pseudocode is confusing. While we're updating this line, can we write out the condition in English?
I believe this should either have a semicolon instead of a comma, or use the word "or".
Once again, an interdiff would be helpful. Thanks!
Comment #33
jstollerThere have been several changes to the Forum module preventing the patch from #29 from applying cleanly. This patch addresses those conflicts, as well as all the changes requested in #32 and a few additional typos I found.
Someone more familiar than I with the workings of this module should look closely at my changes to the preprocess function comments, as requested in #32 points 5 & 6. I did my best, but I'm not 100% confident this is what you want.
Comment #34
jhodgdon#33: forum-clean-up-documentation-1357138-33.patch queued for re-testing.
Comment #36
jhodgdonThis patch looks really good to me! The only thing I would change (assuming it still applies -- sorry for the delay in reviewing it) is:
forum.module - function template_preprocess_forum_icon():
if -> whether in the first two bullet points.
Can someone do a very quick reroll with that (very minor) change? Thanks!
Comment #37
jhodgdonComment #38
jstollerSorry it took me so long to get back to this. Alas, my last patch no longer applies—since the test file was moved, renamed and modified—but I was able to make it work.
This patch includes the requested changes from #36. I'm heading off the grid and into the woods for the next week, so fingers crossed this is the last of it. :-)
Comment #39
jhodgdonLooks good! Committed to 8.x. Ready for backport to 7.x.
Comment #40
tim.plunkettRerolled.
Comment #41
eärendil CreditAttribution: eärendil commentedDocumentation looks good.
Comment #42
jhodgdonThanks! I just committed this patch to 7.x. Another module cleaned up!