CommentFileSizeAuthor
#45 translation-clean-up-documentation-1431632-38-D7.patch14.7 KBjhodgdon
#44 translation-clean-up-one-line-1431632-44.patch609 bytesNROTC_Webmaster
#42 translation-clean-up-one-line-1431632-42.patch914 bytesNROTC_Webmaster
#39 translation-clean-up-documentation-1431632-38-D7.patch14.7 KBNROTC_Webmaster
#36 translation-clean-up-documentation-1431632-36.patch14.66 KBNROTC_Webmaster
#34 translation-clean-up-documentation-1431632-34.patch14.15 KBNROTC_Webmaster
#30 translation-clean-up-documentation-1431632-30.patch14.13 KBNROTC_Webmaster
#30 translation-diff.txt10.69 KBNROTC_Webmaster
#28 translation-clean-up-documentation-1431632-30.patch13.31 KBNROTC_Webmaster
#26 translation-clean-up-documentation-1431632-26.patch12.79 KBNROTC_Webmaster
#24 translation-clean-up-documentation-1431632-24.patch13.3 KBNROTC_Webmaster
#24 translation-diff.txt12.66 KBNROTC_Webmaster
#20 translation-clean-up-documentation-1431632-20.patch12.99 KBNROTC_Webmaster
#20 interdiff.txt8.82 KBNROTC_Webmaster
#17 translation-clean-up-documentation-1431632-17.patch11.78 KBNROTC_Webmaster
#15 translation-clean-up-documentation-1431632-15.patch11.78 KBNROTC_Webmaster
#11 translation-clean-up-documentation-1431632-11.patch11.78 KBNROTC_Webmaster
#9 translation-clean-up-documentation-1431632-9.patch11.74 KBNROTC_Webmaster
#7 translation-clean-up-documentation-1431632-7.patch10.9 KBNROTC_Webmaster
#6 translation-clean-up-documentation-1431632-6.patch11.45 KBNROTC_Webmaster
#4 translation-clean-up-documentation-1431632-4.patch36.68 KBNROTC_Webmaster
#2 translation-clean-up-documentation-1431632-2.patch21.79 KBNROTC_Webmaster
#1 translation-clean-up-documentation-1431632-1.patch13.44 KBNROTC_Webmaster
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

NROTC_Webmaster’s picture

Assigned: Unassigned » NROTC_Webmaster
Status: Active » Needs review
FileSize
13.44 KB

First try at this one. I'm sure I missed a few things but I wanted to get started.

NROTC_Webmaster’s picture

I think this one has everything in it.

jhodgdon’s picture

Status: Needs review » Needs work

Wow, that module had some interesting formatting in it! Thanks!

A few things still need attention:
a)

+ * The node table stores the values used by this module:
+ *  - 'tnid' is the translation set id, which equals the node id
+ *    of the source post.
+ *  - 'translate' is a flag, either indicating that the translation
+ *    is up to date (0) or needs to be updated (1).

This is closer to our list formatting standards, but not quite there:
http://drupal.org/node/1354#lists

b)

/**
- * Menu access callback.
+ * Menu callback: Displays tabs for translation enabled nodes.
  *
- * Only display translation tab for node types, which have translation enabled
- * and where the current node is not language neutral (which should span
- * all languages).
+ * Only display the translation tab for node types, which have a translation
+ * enabled and where the current node is not language neutral (which should
+ * span all languages).
+ *
+ * @param $node
+ *   Node object.
+ *
+ * @return
+ *   TRUE if the translation tab should be displayed, FALSE otherwise.
  */
 function _translation_tab_access($node) {

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.

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
36.68 KB

Thanks 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.

xjm’s picture

Status: Needs review » Needs work

Thanks @NROTC_Webmaster! The cleanup sprint is only for the API documentation, which are the code comments in doblocks, e.g.:

/**
 * Stuff.
 *
 * More stuff.
 */

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.

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
11.45 KB

Ok,

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.

NROTC_Webmaster’s picture

Correcting the last patch.

xjm’s picture

Status: Needs review » Needs work

Thanks @NROTC_Webmaster! That makes it much easier to review. Here's a few other things I found on reading the latest patch:

  1. +++ b/core/modules/translation/translation.moduleundefined
    @@ -70,11 +70,19 @@ function translation_menu() {
    + * Access callback: Checks for access permissions.
    

    Can we make this more specific? Checks for access permissions for what? Based on what?

  2. +++ b/core/modules/translation/translation.moduleundefined
    @@ -205,9 +213,10 @@ function translation_form_node_form_alter(&$form, &$form_state) {
    + * simple links built through the result of ¶
    
    +++ b/core/modules/translation/translation.testundefined
    @@ -447,7 +462,19 @@ class TranslationTestCase extends DrupalWebTestCase {
    +   * @return ¶
    

    Trailing whitespace here.

  3. +++ b/core/modules/translation/translation.moduleundefined
    @@ -436,7 +447,7 @@ function translation_remove_from_set($node) {
    + * Gets all nodes in a translation set, represented by $tnid.
    

    I'd rephrase this as:
    "Gets all nodes in a given translation set."

  4. +++ b/core/modules/translation/translation.pages.incundefined
    @@ -6,10 +6,13 @@
    + * @return
    + *   Translation table.
    

    Can we be a little more specific here? Is it an HTML string? A render array?

  5. +++ b/core/modules/translation/translation.testundefined
    @@ -54,8 +54,10 @@ class TranslationTestCase extends DrupalWebTestCase {
    +   * Creates, modifies, and updates a basic page with a translation.
    +   *
    +   * Creates a basic page with a translation, modifies the basic page outdating
    +   * the translation, and then updates the translation.
    

    I think the second paragraph here is redundant, so we can just remove it.

  6. +++ b/core/modules/translation/translation.testundefined
    @@ -306,14 +316,17 @@ class TranslationTestCase extends DrupalWebTestCase {
    +   * @return
    +   *   Node object.
    

    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.)

  7. +++ b/core/modules/translation/translation.testundefined
    @@ -334,17 +347,19 @@ class TranslationTestCase extends DrupalWebTestCase {
    +   * @return
    +   *   Translation information.
    

    Can we be a little more specific here? What format is the "translation information"?

  8. +++ b/core/modules/translation/translation.testundefined
    @@ -393,7 +408,7 @@ class TranslationTestCase extends DrupalWebTestCase {
    +   * Checks to see if the specified language switch links are found or not.
    

    I'd say "Tests whether the specified language switch links are found."

NROTC_Webmaster’s picture

I 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.

xjm’s picture

Thanks! Couple other small things I noticed reading #9:

+++ b/core/modules/translation/translation.moduleundefined
@@ -2,21 +2,21 @@
+ * The node table stores the values used by this module:
+ *  - tnid: Integer for the translation set id, which equals the node id
+ *    of the source post.
+ *  - translate: Integer is a flag, either indicating that the translation
+ *    is up to date (0) or needs to be updated (1).

The indentation of the list is one space too far in here, I think. Reference: http://drupal.org/node/1354#lists

+++ b/core/modules/translation/translation.moduleundefined
@@ -472,15 +483,14 @@ function translation_node_get_translations($tnid) {
- *   Boolean value.
+ *   A boolean value.

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.

NROTC_Webmaster’s picture

Sorry 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.

NROTC_Webmaster’s picture

Sorry 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.

NROTC_Webmaster’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, translation-clean-up-documentation-1431632-11.patch, failed testing.

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
11.78 KB

I forgot to update before applying the patch. Lets try again.

Status: Needs review » Needs work

The last submitted patch, translation-clean-up-documentation-1431632-15.patch, failed testing.

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
11.78 KB

If this doesn't work I'm going to need some help to figure out whats wrong.

jhodgdon’s picture

The 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. :)

xjm’s picture

Status: Needs review » Needs work

Thanks @NROTC_Webmaster. I think this is getting close. Review for the latest patch:

  1. +++ b/core/modules/translation/translation.moduleundefined
    @@ -70,11 +70,19 @@ function translation_menu() {
    + * Only display the translation tab for node types, which have a translation
    + * enabled and where the current node is not language neutral (which should
    + * span all languages).
    

    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:

    Only displays the translation tab for nodes that are not language-neutral of types that have translation enabled.

  2. +++ b/core/modules/translation/translation.moduleundefined
    @@ -120,8 +128,8 @@ function translation_form_node_type_form_alter(&$form, &$form_state) {
      * Implements hook_form_BASE_FORM_ID_alter().
    

    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?

  3. +++ b/core/modules/translation/translation.moduleundefined
    @@ -206,9 +214,10 @@ function translation_form_node_form_alter(&$form, &$form_state) {
    + * Displays the translation links with language names, if this node is part of
    

    The comma here should be removed as well. I also think we actually don't need the word "the" here (which translation links?)

  4. +++ b/core/modules/translation/translation.pages.incundefined
    @@ -6,10 +6,13 @@
    + * @return
    + *   Translation table array of language, title, status, and operations.
    

    I think this should probably be formatted as a key:value list. Reference: http://drupal.org/node/1354#lists

  5. +++ b/core/modules/translation/translation.testundefined
    @@ -261,15 +260,23 @@ class TranslationTestCase extends DrupalWebTestCase {
    +   * Installs or enables the specified language.
    +   *
    +   * Installs the specified language if it has not been already. Otherwise it
    +   * makes sure that the language is enabled.
    

    I think the second paragraph is probably a bit redundant given the summary. Maybe the summary should say:

    Installs the specified language, or enables it if it is already installed.

    (I checked and this seems to fit in 80 chars.)

I also applied the patch locally and noticed the following:

  • There are still two places missing a blank line between @param and @return items in translation.module (for translation_node_get_translations() and translation_path_get_translations()).
  • translation_form_node_type_form_alter() probably needs a fix similar to the one described above for translation_form_node_form_alter(). An @see to the main form constructor would be good as well.
  • The @file docblock for translation.test should probably say "Tests for the Translation module."
  • "Translation module" should be similarly capitalized in the @file for translation.pages.inc
  • TranslationTestCase is missing a docblock.
  • The summary for 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.

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
8.82 KB
12.99 KB

I 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.

jhodgdon’s picture

RE #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

NROTC_Webmaster’s picture

Would this be appropriate?

/**
 * Page callback: Displays a list of a node's translations.
 *
 * @param $node
 *   A node object.
 *
 * @return
 *   An associative array containing:
 *   -language: String to identify the language used.
 *   -title: String containing the title of the node.
 *   -status: Boolean indicating the status of the node.
 *   -operations: A link to edit the node or add a tranlation.
 *
 * @see translation_menu()
 */
jhodgdon’s picture

Status: Needs review » Needs work

Sorry 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

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
12.66 KB
13.3 KB

Here is the updated patch and the diff from #17

Status: Needs review » Needs work

The last submitted patch, translation-clean-up-documentation-1431632-24.patch, failed testing.

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
12.79 KB

Lets try again.

Status: Needs review » Needs work

The last submitted patch, translation-clean-up-documentation-1431632-26.patch, failed testing.

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
13.31 KB

I think I found the problem.

jhodgdon’s picture

Status: Needs review » Needs work

This 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)

+ * - tnid: Integer for the translation set id, which equals the node id

id -> ID (psychological term vs. short for identifier)

c)

+ * - translate: Integer is a flag, either indicating that the translation
+ *   is up to date (0) or needs to be updated (1).

"Integer is a flag"?? that doesn't seem quite right.

d)

 /**
- * Implements hook_form_FORM_ID_alter().
+ * Implements hook_form_FORM_ID_alter() for node_form().
+ *
+ * @see node_form()
  */
 function translation_form_node_type_form_alter(&$form, &$form_state) {

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:

+ * translation set. If no language provider is enabled "fall back" to the
+ * simple links built through the result of

I think we need a comma after enabled, and I don't think we need the "".

f)

 * @return
- *   Boolean value.
+ *   A Boolean value.
  */
 function translation_supported_type($type) {

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)

  * @return
  *   An array of paths of translations of the node accessible
  *   to the current user keyed with language codes.

This could have better line wrapping (closer to 80 characters)

h)

  /**
-   * Return an empty node data structure.
+   * Creates an empty node.
+   *
+   * @param $langcode
+   *   The language code.
+   *
+   * @return
+   *   An empty node data structure.
    */
   function emptyNode($langcode) {
     return (object) array('nid' => NULL, 'langcode' => $langcode);
   }

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)

   /**
-   * Create a "Basic page" in the specified language.
+   * Creates a "Basic page" in the specified language.
    *
    * @param $title
-   *   Title of basic page in specified language.
+   *   The title of basic page in specified language.
    * @param $body
-   *   Body of basic page in specified language.
+   *   The body of basic page in specified language.
    * @param $langcode
    *   (optional) Language code.
+   *
+   * @return
+   *   A node object.
    */
   function createPage($title, $body, $langcode = NULL) {

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)

  /**
-   * Create a translation for the specified basic page in the specified
-   * language.
+   * Creates a translation for the basic page in the specified language.

There is no antecedent for the first "the" here. It should be "for a basic page".

k)

   * @param $title
-   *   Title of basic page in specified language.
+   *   The title of the basic page in a specified language.

==> in the specified
(same for next param down)

l)

+   * @return
+   *   Translation object

Needs to end in "."

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
10.69 KB
14.13 KB

For 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.

jhodgdon’s picture

RE #29 c) -- the complete doc in that area is:

+ * The node table stores the values used by this module:
+ * - tnid: Integer for the translation set id, which equals the node id
+ *   of the source post.
+ * - translate: Integer is a flag, either indicating that the translation
+ *   is up to date (0) or needs to be updated (1).

The translate one should probalby start with "Integer flag, either...". The "is a" doesn't belong here.

jhodgdon’s picture

Oh, and don't worry about the names you give to patch files too much, as long as they end in ".patch". :)

jhodgdon’s picture

Status: Needs review » Needs work

It looks like you already figured out #31, but the list formatting is not right now:

+ * The node table stores the values used by this module:
+ * - tnid: Integer for the translation set ID, which equals the node ID of the
+ * source post.
+ * - translate: Integer indicating that the translation is up to date (0) or
+ * needs to be updated (1).

The 3rd and 5th lines need to be indented.

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
14.15 KB

#33 corrected.

xjm’s picture

Status: Needs review » Needs work

This 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.)

  1. +++ b/core/modules/translation/translation.moduleundefined
    @@ -473,21 +485,21 @@ function translation_node_get_translations($tnid) {
    + *   An array of paths of translations of the node accessible to the current
    + *   user keyed with language codes.
    

    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.)

  2. +++ b/core/modules/translation/translation.testundefined
    @@ -251,7 +253,7 @@ class TranslationTestCase extends DrupalWebTestCase {
    -   * Reset static caches to make the test code match the client site behavior.
    +   * Resets static caches to make the test code match the client site behavior.
    

    While we're fixing this, let's hyphenate "client-side."

  3. +++ b/core/modules/translation/translation.testundefined
    @@ -333,17 +343,19 @@ class TranslationTestCase extends DrupalWebTestCase {
        * @param $langcode
    -   *   Language code.
    +   *   (optional) Language code.
    

    Is this actually optional? I don't see a default provided for it in the signature. EDIT: Sorry, this is in reference to createTranslation().

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
14.66 KB

For #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.

jhodgdon’s picture

Agreed on client-side. :)

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs review » Patch (to be ported)

This looks good now - committed to 8.x. Time for backport!

NROTC_Webmaster’s picture

jhodgdon,

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?

NROTC_Webmaster’s picture

Status: Patch (to be ported) » Needs review
jhodgdon’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

RE #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!

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
914 bytes

Here 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.

Status: Needs review » Needs work

The last submitted patch, translation-clean-up-one-line-1431632-42.patch, failed testing.

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
609 bytes

I probably should have given it a little more time. Here is the latest.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
FileSize
14.7 KB

OK, I got the patch in #44 into Drupal 8, thanks!!

Uploading the patch in #39 again for D7.

Status: Needs review » Needs work

The last submitted patch, translation-clean-up-documentation-1431632-38-D7.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review

huh? The system message says it failed, but the green bar says it passed...

NROTC_Webmaster’s picture

jhodgdon’s picture

Status: Needs review » Fixed

Thanks! This all looks good for D7. Committed! One more API cleanup done. :)

Automatically closed -- issue fixed for 2 weeks with no activity.