Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
taxonomy.module
Priority:
Minor
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
5 Jul 2014 at 15:22 UTC
Updated:
29 Jul 2014 at 23:43 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
yesct commentedComment #2
yesct commentedjust doing the same as in #1498660: Refactor taxonomy entity properties to multilingual, adding the period to make them "sentences".
Comment #3
yesct commentedadding 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.
Comment #4
yesct commentedComment #5
David Hernández commentedI'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:
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:
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:
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:
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!
Comment #6
yesct commentedok.
@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.
Comment #7
mon_franco commentedPlease find attached the patch to fix the issue mentioned on comment #6.
Comment #8
mon_franco commentedFixed the capitalization and the third person.
Comment #9
mon_franco commentedFixed lines 386 to 389
Comment #10
yesct commentedlooks like the additional comment stuff from @David Hernández in #5 is addressed.
and looks good enough to me.
Comment #11
estoyausenteI check that all @David Hernandez task and the patch and I think that all is correct. :) Congrats @Mon ;-)
Comment #12
alexpottCommitted 4111c2e and pushed to 8.x. Thanks!