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.
Problem/Motivation
Part of meta-issue #1310084: [meta] API documentation cleanup sprint
Proposed resolution
Correct the following in the core locale module:
- Ensure that all
@return
lines are preceded by a blank line. - Ensure that
@see
and@ingroup
lines are always at the end of docblocks. - Ensure that all lines in doxygen blocks are 80 characters or fewer (excluding the terminal newline).
- Ensure that all functions, classes, interfaces, and methods have one-line summaries that are clear, use the correct verb tense, and follow specific standards in http://drupal.org/node/1354.
- Make incidental style and grammar corrections only to those docblocks already being updated.
Remaining tasks
Patch needs to be rolled.
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#48 | 1326618-48-locale-docs.patch | 79.56 KB | Albert Volkman |
#46 | 1326618-46-locale-docs.patch | 83.27 KB | Lars Toomre |
#42 | 1326618-42-locale-docs.patch | 21.85 KB | Lars Toomre |
#42 | interdiff-1326618-39-42.txt | 10.15 KB | Lars Toomre |
#39 | 1326618-39-locale-docs.patch | 21.7 KB | Lars Toomre |
Comments
Comment #1
xenophyle CreditAttribution: xenophyle commentedHi Lars, based on the documentation cleanup issue it looked like you were no longer working on this issue, so I am assigning it to myself. Let me know if that is wrong.
Comment #2
Lars Toomre CreditAttribution: Lars Toomre commentedGo for it... I was not planning on working on it other than creating the issue so that it was worked on.
Comment #3
xenophyle CreditAttribution: xenophyle commentedHere's a patch.
Comment #4
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. In particular, we're not putting in the Path: lines any more (not on Forms either).
Comment #5
jhodgdontagging
Comment #6
xjmAnother thing Gábor pointed out this morning is that a lot of the adjusted class docblocks don't begin with verbs (and they should). Reference: http://drupal.org/node/1354#classes
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedSince there hasn't been activity for 4+ weeks, I'm going to move this back to unassigned.
Comment #8
daniel_j CreditAttribution: daniel_j commentedThe attached patch includes the updates from comment #3 above, and attempts to follow the menu-callback and class docblock changes.
Comment #9
techninja CreditAttribution: techninja commentedI'll go ahead and review this for the sprint.
Comment #10
techninja CreditAttribution: techninja commentedPatch in #8: Alright, this patch is HUGE, but it shows a lot of hard work done documenting returns and params, and doing verb correction and capitalization. Any problems with this patch are far fewer than the number of real fixes it provides. I'd say if we can get one more set of eyes on this we can get it rolling
Comment #11
jhodgdonThanks for the partial review... We actually *do* need someone to go over this very carefully and not just say "it's better than what was there" but "it's correct". The reason is that once the patch is finalized and committed, it's unlikely someone is going to come back and fix the remaining problems, so we are trying to get them all taken care of on the first pass.
So ... what we're looking for is for someone to go over the patched module and verify that everything listed at
#1310084: [meta] API documentation cleanup sprint
has been taken care of, and that in general the documentation in the module is correct and good.
Comment #12
techninja CreditAttribution: techninja commentedUnderstood. I did actually do a rigorous review of the patch, though considering it's extreme length I don't have the time to ensure that it's 100% for all of locale. Perhaps I shouldn't have doubted myself.
If I resubmitted a patch with the two debatable fixes I can remember off the top of my head we'd still be in the same boat of needing another set of eyes to ensure there's nothing else that needs to be added and that my patch isn't corrupted.
Unless anyone else wants to grab it, I'll re-assign to myself when I get back home from Denver and ensure that anything questionable (no matter how small) it dealt with and resubmitted.
Comment #13
jhodgdonThanks for the patch and review! Looks pretty good; there are still a few things to be fixed:
a)
This should say:
CSS for the Locale module for right-to-left languages.
(at least I think that is more consistent with what we've been doing for these)
Similarly:
Should refer to "the Locale module" rather than "locale.module". Check the @file docs in other files too please. :)
b) locale.bulk.inc
First line I think could use another 'a' and a hyphen: ... for a newly-added language.
c) locale.module near the top:
We can get rid of lines like this.
d) locale.module
see item (b) above.
e)
I don't think tihs is actually for locale_languages_edit_form()?
f) (just below)
Needs "the", and technically if -> whether
g) locale.pages.inc now...
First line needs a . at end.
h)
The return description doesn't really tell me what is returned.
i) Top docblock in locale.test has a typo at the end:
fot -> for
j)
That last line should probably be an @see, and it should give the issue URL (issue numbers do not automatically turn into links in documentation).
k)
I do not think this description is accurate, given the name of the function and that it's an exact copy of the previous method getPoFileWithEmptyMsgstr().
l)
exportation -> export
(same should be done in the next docblock too)
m)
The line before a list or sub-list should end in a :. And I think the comma in the first line there should be a :
Comment #14
oriol_e9g- Remaining tasks: h, k, m
- Needs to be improved: e
Comment #15
xjmPatch will also need an update and have hunks moved to #1392962: Clean up API docs for the language module once #1546752: Move negotiation tests to language module is in.
Comment #16
ykhadilkar CreditAttribution: ykhadilkar commentedI planning to working on this issue. So assigning it to myself. Not sure if its the correct process.
Comment #17
ykhadilkar CreditAttribution: ykhadilkar commentedThe attached patch includes the updates from comment #13 above, plus some more relatively small changes.
Comment #18
ykhadilkar CreditAttribution: ykhadilkar commentedComment #19
xjmComment #21
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedykhadilkar,
Do you still plan on working on this? I know you unassigned it from yourself but you seemed a little unsure about the process so I wanted to confirm.
If I'm wrong on any of the items listed please let me know as I'm still learning also.
Here are a few things that I found that still need some work in this module
The following functions need to be updated according to the form standard
locale_translate_import_form()
locale_translate_import_form_submit()
locale_translate_export_form()
locale_translate_export_form_submit()
The @file block needs to be updated for locale.module
Additionally there are a lot of other items in locale.module need to be updated.
The @file in locale.install should be
* Install, update, and uninstall functions for the Locale module.
There is an extra blank line here.
I would change this to "for the Locale module."
If we are changing these would it be possible to put the similar descriptions next to each other to improve readability.
I would keep this as Tests for
I would say Tests the and remove the extra period.
The @See should be @see and this should not end in a period
This should not be changed in the doc cleanup
This should not have two periods
Comment #22
ykhadilkar CreditAttribution: ykhadilkar commentedComment #23
ykhadilkar CreditAttribution: ykhadilkar commentedNROTC_Webmaster thanks for reviewing. The attached patch includes the updates from comment #21 above.
Comment #24
ykhadilkar CreditAttribution: ykhadilkar commentedComment #25
oriol_e9gComment #27
Albert Volkman CreditAttribution: Albert Volkman commentedUpdated this patch so that it applies cleanly. There have been several things removed since this patch, so I have removed those hunks. Here's a summary-
No longer exists:
core/modules/locale/locale-rtl.css
core/modules/locale/locale.css
Moved:
core/modules/locale/locale.admin.inc -> core/modules/system/system.admin.inc
Comment #28
jhodgdonLooks pretty good, thanks!
A few things still need updating in this patch:
a)
First line should follow http://drupal.org/node/1354#forms
Same for locale_translate_export_form() later on in the patch.
b)
Needs , before and
c) @file for locale.module:
Can this be wrapped to as near 80 characters as possible, and re-written with better grammar please? I guess the only thing that jumped out at me is the "as well as" -> "and"
d)
I agree with removing those two lines... how about the one in the middle too? Other core modules do not have section headers like this.
e)
Should be:
Checks whether the locale translates to English.
Also there should be a @return doc?
f)
I think this param got added to the wrong function?
g)
Wrong form function in first line.
h)
Wrong function names in the documentation.
i) As a note, I didn't look through the files to see if other things need to be added to this patch. I just looked through the patch itself. Usually there is a need to clean up docs for the test files in lib/* directories.
Comment #29
Albert Volkman CreditAttribution: Albert Volkman commentedThanks for the review @jhodgdon!
Comment #30
jhodgdon#29: locale_clean_up_documentation-1326618-29.patch queued for re-testing.
Comment #32
jhodgdonLooks like this one needs a reroll again, sorry for the delay in reviewing! I'm trying to get through these...
Comment #33
Albert Volkman CreditAttribution: Albert Volkman commentedRe-rolled.
Comment #34
jhodgdonThanks for all the attention to this issue!!!!
I found one little error in this latest patch: in function locale_translate_english() in locale.module:
+ * Checks whether locale translates to the English.
No "the" :) I fixed this before I committed the patch -- I think it's gone through enough iterations, reviews, and rerolls at this point!
So... I went back and looked through the locale module directory and all sub-directories after this patch was committed. There are still a lot of things in this module that need to be fixed in the next pass through -- so I'm setting this issue back to "active" for Drupal 8.x:
a) locale.pages.inc - has quite a few functions that need first-line attention. I saw at least 4. Note that we have a special standard for theme_* functions at http://drupal.org/node/1354#themeable
b) typo in locale.pages.inc "translatione"
c) locale.module - @file first line is longer than 80 characters. [probably first "the" can be eliminated?]
d) I think we can get rid of the comment in locale.module that says "// Locale core functionality". The previous patch eliminated the // ----------------- but not that comment.
e) function locale() in locale.module needs @return docs.
f) function locale_reset() in locale.module - first line verb tense is wrong. Also locale_library_info_alter(), locale_string_is_safe(), and perhaps other functions [please check through the file].
g) In locale.module I saw at least one place where a blank line was missing between @param / @return sections.
h) _locale_parse_js_file() in locale.module = does not have one-line description.
i) locale.compare.inc - at least 5 verb tense in first line problems, please check through this file.
j) locale_translation_get_projects() in locale.compare.inc - @see should be at end of doc block.
k) locale.bulk.js, locale.admin.js, locale.admin.css - no @file
l) locale.bulk.inc - has verb tense first line problems
m) locale_translate_batch_import() in locale.bulk.inc - should not have blank lines between @param docs.
n) locale.api.php has some line wrapping problems. Note that @code segments can go over 80 characters though!
o) It doesn't look like the tests and other classes in sub-directory lib have been cleaned up -- many need attention.
Comment #35
Lars Toomre CreditAttribution: Lars Toomre commentedThere are a number of Locale Test clean-ups in #1803656: Improve Tests consistency to standards in modules A-M. Reviews there are welcome since it will help clean up the tests for a number of modules (including this one).
In the meantime, I will try to roll a patch that addresses the items in #34 as well as doing a review of where this module now stands from a documentation perspective.
Comment #36
jhodgdonWell, I just did the review. :)
Comment #37
Lars Toomre CreditAttribution: Lars Toomre commentedThe untested patch addresses all of the items a) through n) in #34. I also went through all of the top level files in this module and caught a few additional things (like '(optional) and list formatting). Let's see what the bot thinks.
Comment #38
jhodgdonThe documentation for test classes should eventually be cleaned up on this issue, not on a separate issue... but that's OK, they can wait for a separate patch. No need to file a separate issue though.
Regarding this patch, it looks mostly good, thanks!
A few things before it's ready to commit:
a) Not sure why an extra blank line was added at the top of locale.bulk.inc? We just need one.
b) Also, please don't change *any* code lines, such as:
c) I can't commit patches with ??? in them:
If you need help figuring out what functions' param/return values are, go ahead and make your patch, then mark it "needs work" instead of "needs review" to indicate it isn't ready for review/commit, and ask your questions (or at least say "I'm working on figuring this out, patch isn't ready yet").
d) The list syntax cleanups are great to do, thanks! One thing I noticed is that some cleaned-up list items still don't end in "." , such as:
e) Update functions in locale.install -- those actually should not have verb updates. See
http://drupal.org/node/1354#hookimpl
Comment #39
Lars Toomre CreditAttribution: Lars Toomre commentedThis patch is untested locally and should address all of the issues in #38. I am unable to roll an interdiff because of changes to the Locale module since #37 was rolled.
Sorry for the ???. Those are notes to myself when editing the file that something is needed there. I thought that I had resolved those before submitting #37. I will check for that too in the future.
As for the test files, somebody else can add those changes to this issue. Once this patch in, I will go through and do a detailed review of the php files in locale/lib/Drupal/locale/. On a preliminary look, I saw there issues to clean up there as well.
Comment #40
jhodgdonThis is looking better, thanks! There are still a couple of items to fix:
a) The change in locale.api.php in this patch is not correct -- see
http://drupal.org/node/1354#hooks
b)
- The @return needs to end in .
- Those string typed params/returns... If they default to NULL, then the type can't really be 'string'... I guess it should be string|null?
c)
othrewise is misspelled
d)
thta and avilable are misspelled
e)
I think the previous order is more logical (and in line with our coding standards).
f) You might read through all of the first-line summaries you added and see if they need a/an/the added or have other grammar issues. For instance:
- Returns HTML for translation edit form. ==> for *the* translation...
- Gets an array of available interface translation file. ==> translation file*s*
- Gets array of projects which are available for interface translation. ==> Gets *an* array...
And I also noticed another one that's in the patch file but not modified:
Comment #41
Lars Toomre CreditAttribution: Lars Toomre commentedThanks for the review @jhodgdon in 40.
Regarding a), I did not realize that *.api.php was treated like update_N() functions. I will correct that here shortly. I suspect that I might have made the same mistake in other pending module review patches. I will try to check after I roll an updated patch that addresses #40.
Comment #42
Lars Toomre CreditAttribution: Lars Toomre commentedHere is a revised patch and interdiff from #39 accounting for comments in #40. The interdiff is larger because I noticed and updated a couple of lists to have '(optional)' and '(required)' after the key and colon. I also added some type hinting to the $langcode variables so that they were treated consistently.
Comment #43
Lars Toomre CreditAttribution: Lars Toomre commented#42: 1326618-42-locale-docs.patch queued for re-testing.
Comment #45
Lars Toomre CreditAttribution: Lars Toomre commentedIssue #1809962: Move some locale updates to update.inc for a safe language upgrade. was committed so this will need to re-rolled.
Comment #46
Lars Toomre CreditAttribution: Lars Toomre commentedThis patch builds upon that in #42 and includes a full initial review of all of the files in this module. Given the number of proposed changes, it appears that we never heretofore have reviewed the various classes and tests that have been added to D8.
I suspect that there are some problems in this big patch, but that most of it is worthy of of review and a commit. Once that is done, we can go back through this module and refine the additional changes. I was quite frankly surprised that over 30 files were being changed by this patch.
Comment #47
jhodgdonThanks!
Large patch... I looked through the first part (see below), and found a few issues:
a)
- First line: Gets *the* config...
- I don't understand why you made that locale.config.... into a list item? It's not a list of options.
- The documentation for the two parameters doesn't make sense to me. It's partly because, for instance "a given language" -- what given language? And the second parameter -- is it $language (language object) or $langcode (language code)?
b) This documentation tells me nothing:
"A" language code? What language code?
c) Same file
I don't think this description is accurate, given what this interface actually is. Same with (later in same file):
Also please be consistent in a/the (it should be "the" probably) within one set of new @param docs -- it's very jarring to switch. It's also helpful if you're going to add documentation with "the" in it to say what it's used for, such as "The language code for ...".
d)
Fix list indentation here.
e) Same file
Get -> Gets... and there are other methods that need verb tense attention in this class.
f) PoDatabaseWriter
Last line needs less indentation.
g) same file
Fix list indentation.
h) This file (PoDatabaseWriter) needs some verb tense attention too.
i)
I really don't know why you bother with these types of changes. They don't add a lot to the documentation since anyone can look and see what the default values are. I would only document what the default values are if you are going to add further explanation, like "Defaults to ..., which ...". It just makes these patches bigger and harder to review, and we don't have a standard saying that we should list out every default value.
And in the first example here, adding "An" doesn't really add a lot of value either.
j) StringDatabaseStorage
Uh oh, I can't commit a patch with ??? in it. I stopped reviewing here, as this patch is obviously not done.
Comment #48
Albert Volkman CreditAttribution: Albert Volkman commentedRe-roll of #46 to apply to head. No new updates.
Comment #49
Gaelan CreditAttribution: Gaelan commentedComment #50
jhodgdonThese issues are a lot of work with very little tangible payoff, so I'm closing the rest of them as "won't fix". Your efforts on working on this issue were appreciated... it was just my fault for starting a task that was very difficult to get right.
Let's instead put our effort into fixing and reviewing documentation that is really unclear and/or wrong, and I hope that the people who worked on these issues are not afraid to jump into a more reasonable issue!