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.
Comment | File | Size | Author |
---|---|---|---|
#26 | drupal-cleanup_comments-1912946-26.patch | 25.43 KB | mitron |
#26 | interdiff.txt | 4.16 KB | mitron |
#23 | drupal-cleanup_comments-1912946-23.patch | 25.43 KB | mitron |
#23 | interdiff.txt | 4.16 KB | mitron |
#20 | drupal-cleanup_comments-1912946-20.patch | 24.17 KB | mitron |
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedComment #2
YesCT CreditAttribution: YesCT commentedComment #3
mitron CreditAttribution: mitron commentedThe patch for this has been posted to Translatable strings not self-explanatory / not context-independent. Should I repost it here?
Comment #4
mitron CreditAttribution: mitron commentedVoilà.
Comment #5
mitron CreditAttribution: mitron commentedComment #7
YesCT CreditAttribution: YesCT commentedthe other issue was committed. yay!
@mitron, you can do a git pull --rebase and then create a new patch from that. :)
Comment #8
mitron CreditAttribution: mitron commentedThis is a new patch created from the latest commit.
Comment #9
dawehnerNice cleanup! Did you had a look at the other .views.inc files in core already? We should add new follow ups for them.
Comment #10
YesCT CreditAttribution: YesCT commentedsome 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.
so this is not implementing the hook?
Maybe keep the comment as an explanation after the one line implements?
Or put
inside the function as a comment.
Comment #11
dawehnerups.
Comment #12
mitron CreditAttribution: mitron commentedBut 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:
That does not seem to be how a hook is invoked.
Comment #13
mitron CreditAttribution: mitron commentedCross posted. Did not intend to change the status.
Comment #14
dawehnerI know there is an issue with a patch since months, but I can't find it at the moment.
Comment #15
mitron CreditAttribution: mitron commentedComment #16
mitron CreditAttribution: mitron commentedThe 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?
Comment #17
mitron CreditAttribution: mitron commentedNotes 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()
Comment #18
mitron CreditAttribution: mitron commentedPatch with changes as recommended in #10.
Comment #19
YesCT CreditAttribution: YesCT commentedthis should wrap more, up to 80 chars. (not blocking)
this should have a period at the end of the sentence.
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.
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.
sentence needs period at the end.
[edit: fixed a few typos]
Comment #20
mitron CreditAttribution: mitron commentedPatch with changes from #19
Comment #21
YesCT CreditAttribution: YesCT commentedsimilar 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.
If this adds value (I think it does. Does it?) Then lets keep in in all the parallel places.
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?
Comment #22
YesCT CreditAttribution: YesCT commentedNeeds work to make consistant.
Comment #23
mitron CreditAttribution: mitron commentedEncore une fois. :)
Comment #25
mitron CreditAttribution: mitron commented#23: drupal-cleanup_comments-1912946-23.patch queued for re-testing.
Comment #26
mitron CreditAttribution: mitron commentedRe-upload. Looks like testing stuck after about 40 hours.
Comment #27
YesCT CreditAttribution: YesCT commentedI read through the whole patch. this looks great!
Comment #28
YesCT CreditAttribution: YesCT commentedin a follow-up... check for other files to improve. See @dawehner's comment in #9.
Comment #29
mitron CreditAttribution: mitron commentedFollowup issue created: #1921970: Clean up comments in core .views.inc files
Comment #30
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.