Problem/Motivation

#1498660: Refactor taxonomy entity properties to multilingual has a little out of scope clean up.

Proposed resolution

Separating that out.

Remaining tasks

User interface changes

No.

API changes

No.

Comments

yesct’s picture

Priority: Normal » Minor
yesct’s picture

StatusFileSize
new3.37 KB

just doing the same as in #1498660: Refactor taxonomy entity properties to multilingual, adding the period to make them "sentences".

yesct’s picture

Status: Active » Needs review
StatusFileSize
new3.71 KB
new3.71 KB

adding the period to phrases that are not really sentences ... seems a bit off.
and the comments were labeling sections of the code, without adding any information.
so I just took them out.

corrected a few other comments in this file.

yesct’s picture

Title: clean up comments in taxonomy.views.inc to be sentences » clean up comments in taxonomy.views.inc
David Hernández’s picture

Status: Needs review » Needs work
Issue tags: +Novice

I've reviewed the patch and I agree with the comments you have removed and the ones you kept. Reviewing the rest of the code I found a couple other things that I think are wrong (not all of them related with comments):

Line 17, 247 and 302:

  $data['taxonomy_term_data']['table']['group']  = t('Taxonomy term');

There is an extra space between "['group']" and the equal symbol. This looks like someone copied & pasted the line with the same mistake.

Line 279:

  // @todo This stuff needs to move to a node field since really it's all about
  //   nodes.

I've asked on IRC about this, because the code standard about comments doesn't say (or I haven't found) anything about indentation of multiple lines comments. The answer has been that the second line comment should not have any extra indentation. And Dave Reid has added that, as it's a @todo, probably is better to put it in just one line, even if it's longer than 80 characters.

An extra comment about this, should I create an issue on the code standards to explain this?

Line 306 and line 311:

// links to self through left.parent = right.tid (going down in depth)
// links directly to taxonomy_term_data via tid

This two lines are missing the period at the end of the line.

Line 345 and line 361 are waaay longer than 80 characters. Is there an exception to this rule if the line is composed by a string?

Line 386 to 389:

 * Views integration for taxonomy_term_reference fields. Adds a term relationship to the default
 * field data.
 *
 * @see field_views_field_default_views_data()

The first line is longer than 80 characters and part of it should be moved to the next line. The last line is missing the period at the end.

Line 410, empty line that can be removed.

Line 461, the file doesn't end on an empty line.

I've done this review with mon_franco, to teach her how to do a review. Any commit attribution, should go to her!

yesct’s picture

ok.
@todo does infact use a 2 space indent (as do all @ directives), and should be wrapped at 80 chars. confirmed with jhodgdon in irc.
[edit: examples here https://www.drupal.org/node/1354#todo ]

and let's not fix other coding stuff, and just stick to comments in this issue.

mon_franco’s picture

Status: Needs work » Needs review
StatusFileSize
new4.17 KB
new814 bytes

Please find attached the patch to fix the issue mentioned on comment #6.

mon_franco’s picture

StatusFileSize
new4.17 KB
new814 bytes

Fixed the capitalization and the third person.

mon_franco’s picture

StatusFileSize
new4.56 KB
new620 bytes

Fixed lines 386 to 389

yesct’s picture

looks like the additional comment stuff from @David Hernández in #5 is addressed.
and looks good enough to me.

estoyausente’s picture

Status: Needs review » Reviewed & tested by the community

I check that all @David Hernandez task and the patch and I think that all is correct. :) Congrats @Mon ;-)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 4111c2e and pushed to 8.x. Thanks!

  • alexpott committed 4111c2e on 8.x
    Issue #2298309 by mon_franco, YesCT: Clean up comments in taxonomy.views...

Status: Fixed » Closed (fixed)

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