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
Part of meta-issue #1310084: [meta] API documentation cleanup sprint
Comments
Comment #1
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedFirst try at this one. I'm sure I missed a few things but I wanted to get started.
Comment #2
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedI think this one has everything in it.
Comment #3
jhodgdonWow, that module had some interesting formatting in it! Thanks!
A few things still need attention:
a)
This is closer to our list formatting standards, but not quite there:
http://drupal.org/node/1354#lists
b)
This should say "Access callback" and the first line needs to make clear it is checking access permissions -- currently it sounds like it is actually displaying something. Also it should have @see translation_menu() at the end. See
http://drupal.org/node/1354#menu-callback
c) Quite a few functions still need first-line-verb updates. First lines that start with:
Remove -> Removes
Return -> Returns
Search -> Searches
etc.
See http://drupal.org/node/1354#functions and the meta-issue this is part of. I didn't check (yet) the rest of the functions not touched by the patch, but noticed quite a few places visible in this patch where the verbs had not been updated.
Comment #4
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedThanks for the feedback.
I think I got most of the items you identified. I wasn't quite sure about the verb tense before but this one should be much closer.
Comment #5
xjmThanks @NROTC_Webmaster! The cleanup sprint is only for the API documentation, which are the code comments in doblocks, e.g.:
So, all the changes to inline comments (lines beginning with
//
) are out of the scope of this issue should be removed. (The standard about verb tenses only applies to the docblock summaries. See: http://drupal.org/node/1354#functions.Comment #6
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedOk,
I pulled all of the docblock comment changes out of the previous one so hopefully I didn't miss any of them. Take a look at this one when you get a chance.
Comment #7
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedCorrecting the last patch.
Comment #8
xjmThanks @NROTC_Webmaster! That makes it much easier to review. Here's a few other things I found on reading the latest patch:
Can we make this more specific? Checks for access permissions for what? Based on what?
Trailing whitespace here.
I'd rephrase this as:
"Gets all nodes in a given translation set."
Can we be a little more specific here? Is it an HTML string? A render array?
I think the second paragraph here is redundant, so we can just remove it.
Let's add an article for this. E.g., "A node object." (The same suggestion may apply in a couple other places in the patch.)
Can we be a little more specific here? What format is the "translation information"?
I'd say "Tests whether the specified language switch links are found."
Comment #9
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedI corrected all but #4 and 7. I broke my test site earlier so I'm not quite sure what type they return right now. Here is what I have so far and I will try to get the other two in there tomorrow.
Comment #10
xjmThanks! Couple other small things I noticed reading #9:
The indentation of the list is one space too far in here, I think. Reference: http://drupal.org/node/1354#lists
I learned last week (thanks to @jhodgdon) that the word "Boolean" should always be capitalized (because it's derived from the name Boole). So this should be:
A Boolean value.
Comment #11
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedSorry I have been swamped all week. Take a look at this one and let me know if this is what you were looking for from comments 4 and 7 from before.
Comment #12
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedSorry I have been swamped all week. Take a look at this one and let me know if this is what you were looking for from comments 4 and 7 from before.
Comment #13
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedComment #15
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedI forgot to update before applying the patch. Lets try again.
Comment #17
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedIf this doesn't work I'm going to need some help to figure out whats wrong.
Comment #18
jhodgdonThe patch in #15 didn't apply for me (hunk #8 failed), but the one in #17 does, so hopefully the test bot will get the same result. :)
Comment #19
xjmThanks @NROTC_Webmaster. I think this is getting close. Review for the latest patch:
This may be out of scope for the cleanup, but since we're already changing this text, the comma in this line should be removed. Maybe a clearer phrasing would be:
I think we should say what the BASE_FORM_ID is here? E.g.,
Implements hook_form_BASE_FORM_ID_alter() for node_form().
Is that correct?The comma here should be removed as well. I also think we actually don't need the word "the" here (which translation links?)
I think this should probably be formatted as a key:value list. Reference: http://drupal.org/node/1354#lists
I think the second paragraph is probably a bit redundant given the summary. Maybe the summary should say:
(I checked and this seems to fit in 80 chars.)
I also applied the patch locally and noticed the following:
@param
and@return
items in translation.module (fortranslation_node_get_translations()
andtranslation_path_get_translations()
).translation_form_node_type_form_alter()
probably needs a fix similar to the one described above fortranslation_form_node_form_alter()
. An@see
to the main form constructor would be good as well.@file
docblock fortranslation.test
should probably say "Tests for the Translation module."@file
fortranslation.pages.inc
TranslationTestCase
is missing a docblock.translation_node_overview()
is missing a verb.Also, can you create an interdiff that shows your changes from the most recent version of the patch? You can use the
interdiff
command if you have patchutils installed, or create one with git as part of your patching workflow using the instructions at http://xjm.drupalgardens.com/blog/interdiffs.Comment #20
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedI think I made all of the changes except one.
The summary for translation_node_overview() is missing a verb. - What verb is it missing?
Also I'm not sure how to go about making the interdiff. I'm using tortoisegit and I made one but not really the directions you gave. Take a look and see if it is what you were looking for or not.
Comment #21
jhodgdonRE #20 - translation_node_overview()'s first line says:
Overview page for a node's translations.
Our standard is that all first line function docs start with a verb. This doesn't. Actually, it is a page callback from hook_menu(), so should be documented according to the standards in:
http://drupal.org/node/1354#menu-callback
Comment #22
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedWould this be appropriate?
Comment #23
jhodgdonSorry this slipped my notice.
I don't think #22 is quite right. This function returns a render array, and it should be documented as returning a render array, as in
http://drupal.org/node/1354#menu-callback
Comment #24
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedHere is the updated patch and the diff from #17
Comment #26
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedLets try again.
Comment #28
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedI think I found the problem.
Comment #29
jhodgdonThis looks pretty good! A couple of things could use a little work:
a) When you remove the wrong indentation in the translation.module @file block, you need to rewrap the paragraphs a bit. Some lines are significantly less than 80 characters.
b)
id -> ID (psychological term vs. short for identifier)
c)
"Integer is a flag"?? that doesn't seem quite right.
d)
You don't really need the @see here, since node_form() is referenced in the first line (applies to next docblock too). :)
Oh, but wait! I think this is actually node_type_form() not node_form(), right?
e) Let's fix the punctuation here:
I think we need a comma after enabled, and I don't think we need the "".
f)
Can we explain what the return value is, like "TRUE if translation is supported, and FALSE if it isn't" or something like this?
g)
This could have better line wrapping (closer to 80 characters)
h)
I think this first line is misleading, since when I read it I thought it actually created the node in the database, and it doesn't.
i)
The extra "the"s you added are good, but I think it needs more, such as:
The title of basic page in specified language. ==> The title of the basic page in the specified language.
j)
There is no antecedent for the first "the" here. It should be "for a basic page".
k)
==> in the specified
(same for next param down)
l)
Needs to end in "."
Comment #30
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedFor c) are you saying that it is a bad description or you just don't like the wording?
I messed up the numbering on the last one so hopefully that doesn't create any problems with this one.
Comment #31
jhodgdonRE #29 c) -- the complete doc in that area is:
The translate one should probalby start with "Integer flag, either...". The "is a" doesn't belong here.
Comment #32
jhodgdonOh, and don't worry about the names you give to patch files too much, as long as they end in ".patch". :)
Comment #33
jhodgdonIt looks like you already figured out #31, but the list formatting is not right now:
The 3rd and 5th lines need to be indented.
Comment #34
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commented#33 corrected.
Comment #35
xjmThis looks pretty good! I just noticed three things, two of which are textual issues that predate the patch. (I'd not even mention them except for the third.)
This is a mite confusing; can we add a comma before "keyed" maybe? (While we're changing the line; the text predates this patch, of course.)
While we're fixing this, let's hyphenate "client-side."
Is this actually optional? I don't see a default provided for it in the signature. EDIT: Sorry, this is in reference to
createTranslation()
.Comment #36
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedFor #2 previously it was "client site" and I changed it to "client-side". I'm not sure if that is what you actually wanted or if you misread it, but let me know and I can go either way on it. I think client-side sounds better but I don't want to change it without concurrence.
Comment #37
jhodgdonAgreed on client-side. :)
Comment #38
jhodgdonThis looks good now - committed to 8.x. Time for backport!
Comment #39
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedjhodgdon,
Here is the D7 patch. Please look over this one because there were a few changes between the versions.
One problem I just noticed with the D8 version that wasn't correct by the patch is for addLanguage() in translation.test. The return says "The language code the check." Should I make a one line patch for the D8 that changes it to "The language code to check." or should I leave it as is?
Comment #40
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedComment #41
jhodgdonRE #39, yes we'd better go back and patch D8 with a one-liner, then come back to D7 and review the patch you just made. Thanks for noticing that!
Comment #42
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedHere is the one liner. I hope this will apply but since my local files aren't showing the merge yet there could be a conflict.
Comment #44
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedI probably should have given it a little more time. Here is the latest.
Comment #45
jhodgdonOK, I got the patch in #44 into Drupal 8, thanks!!
Uploading the patch in #39 again for D7.
Comment #47
jhodgdonhuh? The system message says it failed, but the green bar says it passed...
Comment #48
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commented#45: translation-clean-up-documentation-1431632-38-D7.patch queued for re-testing.
Comment #49
jhodgdonThanks! This all looks good for D7. Committed! One more API cleanup done. :)