Follow up for #1779658-52: Translatable strings not self-explanatory / not context-independent and comments #46

Problem/Motivation

comments are not meeting the standards: http://drupal.org/node/1354

Proposed resolution

clean up the comments, removing the un-needed ones, making others complete sentences, and moving inline comments to their own lines, etc.

first, apply the patch from #1779658: Translatable strings not self-explanatory / not context-independent if it is not commented yet, then clean up the files.
(the other issue is actually fixing code, and only touching lines that were changed there. this issue is to clean up the whole file(s).)

Remaining tasks

decide if it makes sense to do all three files in this one issue
make an initial try at clean up and post a patch

User interface changes

No UI changes.

API changes

No API changes.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

Title: Copy of Translatable strings not self-explanatory / not context-independent » Clean up code in comments in comment.views.inc node.views.inc user.views.inc
YesCT’s picture

Title: Clean up code in comments in comment.views.inc node.views.inc user.views.inc » Clean up comments in comment.views.inc node.views.inc user.views.inc
mitron’s picture

The patch for this has been posted to Translatable strings not self-explanatory / not context-independent. Should I repost it here?

mitron’s picture

Voilà.

mitron’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-cleanup_comments-1912946-4.patch, failed testing.

YesCT’s picture

the other issue was committed. yay!

@mitron, you can do a git pull --rebase and then create a new patch from that. :)

mitron’s picture

Status: Needs work » Needs review
FileSize
24.26 KB

This is a new patch created from the latest commit.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Nice cleanup! Did you had a look at the other .views.inc files in core already? We should add new follow ups for them.

YesCT’s picture

+++ b/core/modules/comment/comment.views.incundefined
@@ -11,8 +11,7 @@
-  // Define the base group of this table. Fields that don't
-  // have a group defined will go into this field by default.

+++ b/core/modules/node/node.views.incundefined
@@ -14,13 +14,9 @@
-  // Advertise this table as a possible base table

@@ -33,49 +29,38 @@ function node_views_data() {
-  // This definition has more items in it than it needs to as an example.
...
-      'field' => 'title', // the real field. This could be left out since it is the same.
-      'group' => t('Content'), // The group it appears in on the UI. Could be left out.

@@ -146,14 +127,13 @@ function node_views_data() {
-      'use_equal' => TRUE, // Use status = 1 instead of status <> 0 in WHERE statment

@@ -237,10 +212,6 @@ function node_views_data() {
-  // Define some fields based upon views_handler_field_entity in the entity
-  // table so they can be re-used with other query backends.
-  // @see views_handler_field_entity

@@ -275,8 +246,6 @@ function node_views_data() {
-  // Bogus fields for aliasing purposes.

@@ -439,16 +401,13 @@ function node_views_data() {
-  // For other base tables, explain how we join

some of these comments seem to actually be adding useful information.

I think when we agreed to take out most comments, we were thinking of the ones that were just repeating the code.

+++ b/core/modules/node/node.views.incundefined
@@ -646,7 +580,7 @@ function node_views_data() {
- * Implements hook_preprocess_node().
+ * Alter comments and links based on the settings in the row plugin.

so this is not implementing the hook?

+++ b/core/modules/user/user.views.incundefined
@@ -385,7 +359,7 @@ function user_views_data() {
- * Allow replacement of current userid so we can cache these queries
+ * Implements hook_views_query_substitutions().

Maybe keep the comment as an explanation after the one line implements?

Or put

- * Allow replacement of current userid so we can cache these queries

inside the function as a comment.

dawehner’s picture

Status: Reviewed & tested by the community » Needs review

ups.

mitron’s picture

Status: Needs review » Reviewed & tested by the community

But the other comments don't really add anything beyond explaining how to interact with views or the function of a hook. Does that documentation really belong in module.views.inc files?

The function node_row_node_view_preprocess_node() does not seem to be a proper hook. There is the following invoking code in views.module:

// Allow to alter comments and links based on the settings in the row plugin.
  if (!empty($vars['view']->style_plugin->row_plugin) && get_class($vars['view']->style_plugin->row_plugin) == 'views_plugin_row_node_view') {
    node_row_node_view_preprocess_node($vars);
  }

That does not seem to be how a hook is invoked.

mitron’s picture

Status: Reviewed & tested by the community » Needs review

Cross posted. Did not intend to change the status.

dawehner’s picture

The function node_row_node_view_preprocess_node() does not seem to be a proper hook. There is the following invoking code in views.module:

I know there is an issue with a patch since months, but I can't find it at the moment.

mitron’s picture

Assigned: Unassigned » mitron
mitron’s picture

The following comment appears in node.views.inc.

// Define some fields based upon views_handler_field_entity in the entity
// table so they can be re-used with other query backends.
// @see views_handler_field_entity

The term "views_handler_field_entity" does not appear in any other file in the current version of D8. Where is the destination of the @see?

mitron’s picture

Notes from IRC:
[1:15pm] timplunkett: mitron: that's just an out of date doc
[1:16pm] mitron: Yeah. I'm trying to update it. I took it out but YesCT told me it had useful information so I'm trying to figure out what to put in its place.
[1:17pm] timplunkett: mitron: @see \Drupal\views\ViewsDataCache::processEntityTypes()

mitron’s picture

Patch with changes as recommended in #10.

YesCT’s picture

Status: Needs review » Needs work
+++ b/core/modules/node/node.views.incundefined
@@ -15,8 +15,11 @@
+  // Define the base group of this table. Fields that don't
+  // have a group defined will go into this field by default.

this should wrap more, up to 80 chars. (not blocking)

+++ b/core/modules/node/node.views.incundefined
@@ -15,8 +15,11 @@
+  // Advertise this table as a possible base table

this should have a period at the end of the sentence.

+++ b/core/modules/node/node.views.incundefined
@@ -49,6 +52,7 @@ function node_views_data() {
+  // This definition has more items in it than it needs to as an example.

this is added back in.
I think the inline comments on the extra lines, which were removed, helped to identify which were extra.
Either keep those, but on their own lines, or just put it here. Like:

As an example, this definition has more items than it needs; field and group are extra.

+++ b/core/modules/node/node.views.incundefined
@@ -127,7 +131,7 @@ function node_views_data() {
-      'use_equal' => TRUE,
+      'use_equal' => TRUE, // Use status = 1 instead of status <> 0 in WHERE statment

wait, is this interdiff backwards? no, I checked and it looks from the patch like the interdiff is correct. this was probably added back because it adds info. but the comment needs to be on it's own line.

+++ b/core/modules/node/node.views.incundefined
@@ -401,6 +410,7 @@ function node_views_data() {
+  // For other base tables, explain how we join

+++ b/core/modules/user/user.views.incundefined
@@ -360,6 +360,8 @@ function user_views_data() {
+ * Allow replacement of current userid so we can cache these queries

sentence needs period at the end.

[edit: fixed a few typos]

mitron’s picture

Status: Needs work » Needs review
FileSize
2.27 KB
24.17 KB

Patch with changes from #19

YesCT’s picture

+++ b/core/modules/comment/comment.views.incundefined
@@ -11,8 +11,7 @@
-  // Define the base group of this table. Fields that don't
-  // have a group defined will go into this field by default.

@@ -387,14 +368,9 @@ function comment_views_data() {
   $data['node_comment_statistics']['table']['group']  = t('Content');

+++ b/core/modules/node/node.views.incundefined
@@ -14,13 +14,12 @@
+  // Define the base group of this table. Fields that don't have a group defined
+  // will go into this field by default.

@@ -420,16 +400,10 @@ function node_views_data() {
-  // Define the base group of this table. Fields that don't
-  // have a group defined will go into this field by default.
...
   $data['node_revision']['table']['group']  = t('Content revision');

@@ -618,21 +572,14 @@ function node_views_data() {
-  // Define the base group of this table. Fields that don't
-  // have a group defined will go into this field by default.

+++ b/core/modules/user/user.views.incundefined
@@ -11,10 +11,6 @@
-  // Define the base group of this table. Fields that don't
-  // have a group defined will go into this field by default.

@@ -324,13 +304,9 @@ function user_views_data() {
   $data['users_roles']['table']['group']  = t('User');

@@ -358,10 +334,8 @@ function user_views_data() {
   $data['role_permission']['table']['group']  = t('User');

similar to the node views inc, I think this adds value. (Does it?) Leave in and word wrap to 80 chars. Add where missing for consistency.

+++ b/core/modules/node/node.views.incundefined
@@ -439,16 +413,14 @@ function node_views_data() {
-  // For other base tables, explain how we join
+  // For other base tables, explain how we join.

@@ -618,21 +572,14 @@ function node_views_data() {
-  // For other base tables, explain how we join

+++ b/core/modules/user/user.views.incundefined
@@ -324,13 +304,9 @@ function user_views_data() {
-  // Explain how this table joins to others.

If this adds value (I think it does. Does it?) Then lets keep in in all the parallel places.

+++ b/core/modules/node/node.views.incundefined
@@ -535,20 +495,18 @@ function node_views_data() {
-      'use_equal' => TRUE, // Use status = 1 instead of status <> 0 in WHERE statment
+      'use_equal' => TRUE,

if this comment adds value (I think so) similar to the other instance, let's keep this comment, but on it's own line.

*note* As someone not familiar with this comments code, those seem to add value. What do others think?

YesCT’s picture

Status: Needs review » Needs work

Needs work to make consistant.

mitron’s picture

Status: Needs work » Needs review
FileSize
4.16 KB
25.43 KB

Encore une fois. :)

Status: Needs review » Needs work
Issue tags: -Novice, -VDC

The last submitted patch, drupal-cleanup_comments-1912946-23.patch, failed testing.

mitron’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +VDC
mitron’s picture

Re-upload. Looks like testing stuck after about 40 hours.

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

I read through the whole patch. this looks great!

YesCT’s picture

in a follow-up... check for other files to improve. See @dawehner's comment in #9.

mitron’s picture

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

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