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 |
---|---|---|---|
#43 | poll_api_cleanup-1355712-43.patch | 682.88 KB | izus |
#43 | interdiff-1355712-43.txt | 707 bytes | izus |
#44 | poll_api_cleanup-1355712-44.patch | 17.37 KB | izus |
#44 | interdiff-1355712-44.txt | 707 bytes | izus |
#41 | interdiff.txt | 1.27 KB | izus |
Comments
Comment #1
surendramohan CreditAttribution: surendramohan commentedClaimed Poll Module #1355712: Clean up API docs for Poll Module
Comment #2
jhodgdon@surendramohan: Do you still plan to work on this? If so, please respond. If not, we'll unassign it so someone else can. Thanks!
Also, tagging.
Comment #3
chertzogI will take this over. Should have a patch tomorrow.
Comment #4
chertzogFirst run.
Comment #5
jhodgdonThanks for the patch! Looks like a good start. A few things need to be addressed:
a)
Needs to start with a verb, or perhaps use the standard for callback functions:
http://drupal.org/node/1354#callbacks
b)
Blank line is needed before the @see.
c)
This is not actually a form submission handler -- wrong args. Need to figure out what it is and document that instead.
d)
Don't add this extra blank line.
e)
Why are all these @see lines needed here? Also this needs @param docs, and probably @return.
f)
Needs @param docs for $node, $block.
g)
Not a good one-line description here. A preprocess function doesn't "theme" anything.
h) There are quite a few other functions that need @param and/or @return docs in poll.module.
i)
In the first line, wrong verb tense, and instead of "the poll_results theme hook" it should say "poll-results.tpl.php".
j)
The @see is not necessary, as it's referenced in the first line.
k) poll.pages.inc now...
The @return here needs to say whether it's returning a string or a renderable array. "a page" is ambiguous. See
http://drupal.org/node/1354#menu-callback
Same applies to the other page callbacks in this file.
l) There is a lot to clean up in poll.test, which is missing from this patch.
m) poll.module : poll_block_latest_poll_view() - needs to be fixed up and included in this patch.
Comment #6
gapa CreditAttribution: gapa commentedI have corrected problem from a), b), d), e), f), g), i), k)
And not (yet) from c), h), l), m)
This is my first time to provide patch for documentation. Please review it and give me feedback so I could continue with this in proper direction. I don't know documentation standars so good but will learn them. I have a feeling that more can be done than just notes that were given to me to create good documentation.
br,
Gapa
Comment #7
xjmMarking for review.
Comment #9
xjmThanks for your work on this @gapa. It appears that the patch was created with NetBeans and is not compatible with git. See http://drupal.org/node/707484 for information on creating git patches.
Comment #10
gapa CreditAttribution: gapa commentedAhhh. I uploded wrong one. Hope this works better. Done with git diff.
Thanks
Comment #11
gapa CreditAttribution: gapa commentedMarking for review.
Comment #13
jhodgdonThe poll.module part of the patch apparently needs a reroll. This part doesn't apply:
Comment #14
gapa CreditAttribution: gapa commentedLet's try again. Thanks for your help with this.
Comment #16
jhodgdonThe failed test is in the Poll module, so there is likely some actual problem with this patch...
Comment #17
xjmThese changes do not belong. The patch should only include doxygen changes. Please review patches carefully before you upload them. :)
Comment #18
gapa CreditAttribution: gapa commentedThe last two changes I probably made. Don't know actually why.
But the first four changes I most definitly didn't do. But I probably had older version of drupal8. It actually was "
<article ...
" at some point but went back to "<div ...
". Convert poll tpls and markup to HTML5I wasn't expecting this to be so complicated. :) But thanks for your help and I hope that this time it will work.
Comment #19
xjmIt was probably a result of having a branch that was not up-to-date and doing a diff. (So your branch was missing some changes that were in the 8.x branch, and thus when you created the diff it reverted those changes.) One way to avoid those issues is to use git's rebase functionality in the local repository while you are working on your patch. :) I know I still mess it up sometimes anyway, which is why I always read over patch files when I submit them to make sure all the changed lines are changes I actually meant to make!
Comment #20
jhodgdonThanks for the new patch! I gave it a review, and it still needs quite a bit of work... First, some general observations:
1. There were a LOT of @see lines added. While this is not a bad idea, I am not sure all of them are necessary. I didn't check them over, but generally we want @see to related functions that might not be obvious, but not @see to all the functions that a given function calls or that call this function.
2. The standards for writing documentation are at http://drupal.org/node/1354
3. Usually we don't want to remove information that is there -- just reformat it correctly.
Specific comments on the patch (I didn't apply the patch to see if there were other things in the Poll module directory that needed to be changed -- these comments only apply to what I could see in the patch file):
a)
- The first line is not good grammar, and it doesn't follow the standards at http://drupal.org/node/1354#menu-callback
- The param description shown here also isn't good grammar. How about "TRUE if the user can view votes, and FALSE if the user cannot view votes"?
- The return value is just wrong. This is an access callback. It doesn't return a render array at all.
b)
This needs to follow the standards for form generating functions at http://drupal.org/node/1354#forms
c)
- First line should say "on poll_form()" not "on a poll node form", I think? Or whichever form function it is for.
- The description should have JavaScript written out, not abbreviated as JS.
d)
The first line does not look right to me. It doesn't have the right arguments to be a form submission handler.
e)
- First line: verb tense is wrong.
- Other changes here: The original was better. There is no reason to take away the parameter type documentation that was added in another patch.
f)
"String of choices" is not very good documentation.
g)
- The @ingroup should be moved to the end -- see http://drupal.org/node/1354#forms
- $block parameter -- description needs some a/the added.
h)
Do not include an @see for poll_view_voting() -- that is taken care of by the first line description. The same applies to the submission handler just below.
i)
It's pre-processing, not processing. And the @see is not necessary, as above.
j)
- Wrong verb tense in first line, and it should be consistent with other pre-process functions.
- Why is the information about inputs removed? It should actually list @param $variables, and then list the array elements present as a list.
k) poll.pages.inc now...
The return value should probably say "HTML for the page".
l)
Here the return value should specify whether it's returning a render array or HTML.
m)
I thought this was very confusing. The first line made me think it was returning results for multiple polls, but I think it is for just one poll node? And the tab is called both "tab" and "page". The return value also needs to tell me whether it's HTML or a render array.
Comment #21
batigolixThis is my first attempt at contributing to cleaning up API docs for core, so be gentle with your feedback ;)
I deliberately chose this issue to start as it seemed rather manageable. It also seemed to me that my possible errors are less critical in the poll module then elsewhere as the future of poll in core is uncertain ;).
I spend the largest part of the day learning to do this (api docs, doxygen, reroll patch), and struggled especially with applying the patches in this issue to my drupal 8 installation. I hope the patch has been generated correctly. It seems to contain both the changes from patch 18 & mine.
I applied all of Jennifer's comments from #20 and made minor style changes.
Crossing fingers
Comment #22
batigolixSet status to needs review
Comment #23
jhodgdonThanks! I'll review this sometime soon. Meanwhile, batigoix - the other thing you should do when you take over is assign the issue to yourself. :)
Comment #24
batigolixyes, that was on my todo list (that i never look at) :)
Comment #25
jhodgdonThanks for the patch! Rather than trying to see if the previous review issues were addressed, I started over reviewing this patch from scratch. It looks pretty good, but there are a few things that need to be fixed:
a) poll.module
Actually, the previous documentation block here was correct. This is an implementation of hook_form(), and not a form constructor. The @ingroup forms line should be removed also. I also can't see how this is related to poll_teaser(), so that should be removed... We probably don't need the rest of the @see lines either, but I guess they don't hurt anything (they are actually related).
b) poll.module
This section is over-agressively wrapped -- it should go as close to 80 characters as possible. Also there are a number of extra spaces at the ends of lines (most text editors for programming allow you to automatically remove these spaces or at least to show them).
c) Just below that, again I don't see how poll_teaser() is related to this function, so that @see (at least -- maybe all of them?) should be removed. The previous patch author seems to have added that @see poll_teaser() in a lot of places -- probably all can be removed. And generally, most of the @see lines could be removed -- see note #1 in comment #20 above. It's really not helpful to have so many see also lines...
d) This see from a few places in poll.module points to a function that does not exist:
e)
We consider module names to be proper nouns, so this should be "the Poll module".
Comment #26
jhodgdonI also took a look at the other files in core/modules/poll that are not included in this patch. There are a few more things to be fixed:
1. Item (e) from my comment above (proper noun for module) needs to be applied to the file header in poll.install.
2. I also just noticed that in the patch, poll.module :: _poll_menu_access() is documented as a page callback. But it's actually an access callback.
3. The tests in core/modules/poll/lib/Drupal/poll/Tests need documentation updates (classes and methods are incorrectly documented or completely undocumented).
Comment #27
batigolixnew attempt. incorporated your feedback
Comment #28
jhodgdonLooks pretty good! A couple more fixes and I think we'll be done here:
1. Item (e) from #25 still isn't fixed:
Should be "the Poll module".
2.
SInce the name of this class is PollTestBase, I think it's actualy a base class for the poll tests, not a test itself?
3.
- Test -> Tests
- This also needs to be done for PolLTokenReplaceTest.php
4. There are a couple more methods in test classes that don't have documentation:
PollTestBase::_generateChoices()
PollVoteTest - class and ::testPollVote() method
5. This method from PollVoteCheckHostnameTest.php needs a bit of fix-up (one line summary & correct verb tense):
Comment #29
batigolixnew attempt
this incorporates the comments from #28
Comment #30
jhodgdon#29: poll_api_cleanup-1355712-29.patch queued for re-testing.
Comment #32
kid_icarus CreditAttribution: kid_icarus commentedRe-rolled, applies cleanly at 96614a6
Comment #33
jhodgdonThis is getting closer, thanks! A couple more things to fix:
a)
missing . at end
b)
Needs serial comma before "and".
c)
I don't think we want to remove that blank line? There are a couple of places that is done in this patch. We normally do want a blank line between methods in a class.
d)
Summary needs to be one line. IP should be all-caps.
e)
Needs serial comma before and
f)
Should be:
Implements template_preprocess_HOOK() for poll-results.tpl.php.
g)
We don't add submission handlers to @ingroup forms.
Comment #34
kid_icarus CreditAttribution: kid_icarus commentedRe-rolled. Applies cleanly at e4c2a57.
Comment #36
kid_icarus CreditAttribution: kid_icarus commentedThanks for the review @jhodgdon! Addresses #33.
Comment #37
kid_icarus CreditAttribution: kid_icarus commentedWhitespace cleanup from the last patch.
Comment #38
jhodgdonSorry again for the delay in reviewing these patches... This looks mostly great, thanks! A couple of things to fix before I can commit this:
a)
The @ingroup needs to be put back here, and probably the @see lines too?
b)
There should be an @see to the submission handler from the validation handler, and vice versa. It's also not necessary (or advisable) to have @see poll_view_voting() if poll_view_voting() is mentioned in the first line.
c)
Should say "Implements template_preprocess_HOOK() for poll-vote.tpl.php".
Other than that, looks great, thanks!
Comment #39
izus CreditAttribution: izus commentedHello,
here is a new version of the patch with recommanded modifications of #38.
hope it helps
Comment #40
jhodgdonCan you make an interdiff? http://drupal.org/node/1488712
Comment #41
izus CreditAttribution: izus commentedHi,
Here is the interdiff corresponding the the last patch in #39.
Comment #42
jhodgdonOK, this patch is almost there! The only thing I see now that needs to be fixed is that @ingroup and @see need to be at the end of documentation blocks (@ingroup last) -- applies to poll_view_voting().
Comment #43
izus CreditAttribution: izus commentedhi,
here is the corresponding patch and interdiff.
EDIT : please forget that patch, the interdiff is ok but the patch is not anly patching the poll module ! my bad !
Comment #44
izus CreditAttribution: izus commentedComment #45
jhodgdonThanks all! Committed this patch in #44 to 8.x.
I took a look at the files in the Poll module and sub-directories, and I think we now need a follow-up to address the following remaining issues:
a) poll-vote.tpl.php - missing a header before the list saying "Available variables:" -- see http://drupal.org/node/1354#templates - and actually the poll-results.tpl.php file is a bit non-standard there too.
b) Both of these templates need a change to the first line of the @file too, see that link in (a).
*** In poll.module:
c) _poll_menu_access() should have @see to poll_menu().
http://drupal.org/node/1354#menu-callback
d) poll_more_choices_submit() - this is a submission handler for the add another choice button in poll_form(); needs to say that in the documentation, and see:
http://drupal.org/node/1354#forms
e) poll_choice_js() - first line needs attention... this is an Ajax form callback, so it could say something like "Ajax callback for poll_form(): Adds new choices..."
f) poll_node_form_submit() - first line needs attention - this is an entity builder callback for poll_form(), so it could say "Entity builder callback for poll_form: Saves the poll choices and builds a teaser."
g) poll_view_voting() - blank line needed between @param and @see sections, and needs @see to its validation function.
h) poll_view_results() - needs @param/@return documentation.
*** Test files:
i) PollVoteCheckHostnameTest.php - missing class docblock.
j) PollTestBase::pollCreate() -- @return says "id" instead of "ID".
k) A few of the methods in PollTestBase are missing param/return documentation.
Comment #46
Lars Toomre CreditAttribution: Lars Toomre commentedNote that #1811880: Add missing type hinting to Poll module docblocks is currently being refined to add type hinting to the appropriate @param and @return directives.
It is also picking up those docblocks with missing @param and/or @return directives. Not sure yet whether it captures all of those noted in #45. I will check once that issue is near ready to RTBC.
Comment #47
alexpottPoll has been removed from core
Comment #48
jhodgdonWell, we still need a Core issue for 7.x, and since this was a Core initiative, I'd prefer to keep this issue for Core. (The patch above in #44 needs to be backported, and then additional issues addressed as outlined in #45.)
If the maintainers of the used-to-be-Core Poll module want to clean up the API docs, maybe they can file another issue?
Comment #49
jhodgdonWe're not working on these issues any more, so I'm setting back to 8.x / long ago fixed.