Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
documentation
Priority:
Normal
Category:
Bug report
Assigned:
Issue tags:
Reporter:
Created:
23 Oct 2015 at 06:37 UTC
Updated:
15 Nov 2015 at 22:44 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
krknth commentedComment #3
krknth commentedComment #4
jhodgdonNo patch yet, so just "active" not "needs work".
Comment #5
krknth commentedAdded patch
Needs review.
Comment #6
jhodgdonLooks fine, thanks!
Comment #7
blackra commentedAwesome work, krknth.
xjm has asked for this issue to be combined with four other similar issues - it makes her job much easier :-)
A combined patch is attached.
Comment #8
blackra commentedFixed a typo in the patch numbering.
Comment #9
rhett.prichard commentedVerified patch [#2599524-8.patch] from [#2599524] by reapplying all the children's patches of this issues's parent. git diff showed no differences.
Comment #10
jhodgdonOh, that was my fault (partly). I suggested to krknth in IRC that the patches be "not too big and not too small" rather than all one big huge issue. So they were split up... maybe slightly smaller than they should have, but it was my suggestion.
Anyway, thanks for the combined patch. I actually prefer smaller patches for reviewing, but that's OK. :)
There are 3 problems with the docs in this patch, which are out of scope for this issue and should be addressed in a separate issue:
Typo:
specifies -> specified
exists => exist
Sigh. This @param and @return need to have docs added to them. Having @param and @return with only a type and no documentation is not OK.
Comment #11
krknth commented@jhodgdon : created separate issue
#2601282: Improve comments in \Drupal\Core\Entity\Query\Sql\Tables
Comment #12
xjmThanks @krknth!
Comment #14
xjmRegarding splitting up the patches, I agree for changes that actually involve different patterns of work or new work, e.g. for #2594909: Missing @return tags in function/method comments in the core/modules folder it would not make sense to try to write lots of missing
@returndocs in one patch since each one would be different. But for something like this where the changes are one or two of the same kinds of simple fixes (just moving lines around) throughout the whole file, it's preferable (and more fair in terms of issue credit) to have one 28K patch rather than five 5K patches or whatever. On the other hand, over a certain number of lines of code, the patch does become unreviewable. I remember reading some research about this but can't locate it. (See my related comment on #2572635-14: Fix 'Drupal.Commenting.DocComment.LongFullStop' coding standard). We have a long-outstanding todo to document such recommendations more clearly; an old draft is in https://docs.google.com/document/d/1zYDnu45djDBcU87sNgKk15C5XDBYzNHvahoq....Note that as far as we could tell when looking into this at BADCamp, there is not yet a coder rule to check for the order of these documentation sections, meaning there's not an existing child of #2571965: [meta] Fix PHP coding standards in core, stage 1 that would fix this as far as I can see. There might be a rule for blank lines between each @whichever grouping; that I didn't check for. A followup would be to patch coder to add rules for the things fixed here (canonical ordering of documentation sections and blank lines separating them).
In any case, committed and pushed to 8.0.x. Thanks everyone to getting this issue sorted out consistently!
Comment #15
blackra commentedThe current coder module does not appear to check this and I could not find an existing issue on this subject so I have created #2605986: Missing check for ordering of comment tags.