Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This is simply a forward port of the docblock from #1302228: field_has_data() returns inconsistent results – possible data loss.
Comment | File | Size | Author |
---|---|---|---|
#62 | node_test_docblock-1540094-62.patch | 3.11 KB | bdgreen |
#58 | node_test_docblock-1540094-58.patch | 3.61 KB | bdgreen |
#56 | node_test_docblock-1540094-56.patch | 3.45 KB | bdgreen |
#54 | node_test_docblock-1540094-54.patch | 2.47 KB | bdgreen |
#52 | node_test_docblock-1540094-52.patch | 16.52 KB | bdgreen |
Comments
Comment #1
xjmComment #2
xjmGood patch for a novice to review. :)
Comment #3
jhodgdonThis is (or at least should be) part of the API Cleanup for Node issue right?
Oh, I see that one is already on to 7.x. What happened here -- why is this patch needed? Did we miss it or was it added recently?
Also, I see that node.test has several other classes without doc blocks. Perhaps we should reopen the other issue?
#1313980: Clean up API docs for node module
for Drupal 8?
Comment #4
xjmWell, this class was added as part of a patch committed long after the node.module cleanup was committed to D8:
#1373142: Use the Testing profile. Speed up testbot by 50%
So unrelated to any other classes that were missed in the cleanup.
Edit: version correction.
Comment #5
xjmAlright, @jhodgdon and I discussed this in IRC, and we agreed to expand this issue to cover the other missing docblocks in the file. Anyone that likes is welcome to work on this. :)
Comment #6
totten CreditAttribution: totten commentedThere are currently 37 test classes in D8's node module, and most have docblocks. To get a specific listing of files/classes/methods that need them, see:
http://drupal.org/node/1359786#comment-6286462
Comment #7
Noe_ CreditAttribution: Noe_ commentedAttached is a patch for review.
Comment #8
xjmThanks @Noe_! Remember to set the status to "Needs Review" when you update the patch. :)
Comment #9
jhodgdonThis patch has non-documentation changes in it, such as:
Please remove those, and stick to the issue we are dealing with, thanks!
Comment #10
Noe_ CreditAttribution: Noe_ commentedRemoved all the code improvements, and I just stuck to documentation issues.
Except setUp() and getInfo() functions because @xjm told me not to document those.
Comment #11
jhodgdonThanks for the new patch! It's a great start. A few things to address:
a)
Wrong verb tense -- see http://drupal.org/node/1354#functions -- this applies to the other doc blocks in the patch too.
b)
If you are going to fix this line, you could also change "login" (an adjective) to "log in" (a verb).
c)
Wrapping - try to put as much on the first line as possible without going over 80 characters.
d)
Grammar: Change "test if" to "test whether" throughout the patch.
e)
This needs to start with a verb.
f)
The added lines don't make much sense to me, and "determing" is misspelled. Also I don't think we need that text repeated twice in the documentation.
Comment #12
Noe_ CreditAttribution: Noe_ commentedI suppose this needs to be:
Tests whether, instead of test whether.
I did that because the first line of documentation need to be limited to 80 characters. However I thought I couldn't just leave the rest out, so I copied it again but this time with the full line.
If you have a better way of doing this please let me know.
Comment #13
jhodgdonYes, "Tests whether..." is correct.
Regarding your second question about repeated text, I would edit the second paragraph so that it doesn't contain the same information as the first line. Maybe start with "Also"?
Comment #14
pesgyo CreditAttribution: pesgyo commentedI'm working on it today.
Comment #15
pesgyo CreditAttribution: pesgyo commentedI tried to correct the errors posted by @jhodgdon and I made some other changes. This is my first patch, I'm sorry if I made some mistake.
Comment #17
pesgyo CreditAttribution: pesgyo commentedComment #18
junedkazi CreditAttribution: junedkazi commented#15: node-test-docblock-1540094-15.patch queued for re-testing.
Comment #20
killtheliterate CreditAttribution: killtheliterate commentedRe-rolled. Patch #15 assumes patch #10 has been applied already. Applies cleanly at ea1b955.
Comment #21
jhodgdonLet's leave the in-code // comments out of this issue. The fixes are a bit difficult to review, and not all of them are correct. For instance:
comments should end in . not ; so this is only a partial fix of this comment.
Reviewing the added/corrected documentation blocks, it looks like the comments in #11 were not addressed.
Some more comments, which might duplicate #11:
a) I found two that still have the wrong verb tense:
b) And this one doesn't even start with a verb:
c) This line has a spelling error:
I stopped at this point because I realize #11 had not been addressed.
Comment #22
dermarioI will work on this on sunday, as part of Badcamp switzerland. There are mentors that may teach me how to perform automated codestyle checks.
Comment #23
wryz CreditAttribution: wryz commented#20: node-test-docblock-1540094-20.patch queued for re-testing.
Comment #25
dermarioI am not sure what to do now. It seems that most of the comments are fixed in D8. Is it just the backport to D7 we need here?
Comment #26
jhodgdonHm. Maybe the first thing to do would be to review the latest patch that was attached before, and the comments that came after, and see if anything was missed -- if so, make a Drupal 8 patch.
And if not, check Drupal 7 to see how that is with regards to those same fixes. If it's fine, mark this issue as fixed. If not, change the version to Drupal 7 and make a patch.
This sometimes happens with documentation cleanup -- sometimes it gets fixed on a different issue. Thanks!
Comment #27
dermarioI checked the D8 node module tests for "determing" and found 1 more occurance. Afterwards i searched for " * Test " and corrected one occurance where the comment started with a Noun.
Comment #29
dermario#27: node-test-docblock-1540094-27.patch queued for re-testing.
Comment #30
jhodgdonThe NodeSaveTest::testDeterminingChanges() documentation block should start with a single-line summary. If additional information is needed, it should be after a paragraph break.
Comment #31
rootworkI've updated the patch from #27 with a new single-line summary and longer paragraph for NodeSaveTest::testDeterminingChanges().
Comment #32
rootworkMeant to tag this as work done during the global sprint.
Comment #33
xjmTrailing whitespace. :)
Looks like a straightforward cleanup otherwise!
Comment #34
jhodgdonComment #35
rootworkRemoved the trailing space.
Comment #36
jhodgdonLooks good, I'll get it committed soon. Thanks!
Comment #37
jhodgdonThere's another issue marked "avoid commit conflicts" that also touches one of these files, so I'll wait to commit this until #1380720: Rename entity "bundle" to "subtype" in the UI text and help is done.
Comment #38
xjm#35: node-test-docblock-1540094-35.patch queued for re-testing.
Comment #42
xjm#35: node-test-docblock-1540094-35.patch queued for re-testing.
Comment #43
xjmComment #44
jhodgdonThe small patch in #35 has been committed to Drupal 8.x.
I think that there are still issues identified in previous patches (the current one started in #27) and comments that have not been patched/committed yet. So let's leave this issue at Drupal 8.x until someone can verify that the patches/comments above have been addressed.
Comment #45
bdgreen CreditAttribution: bdgreen commentedReviewed:
#11
a) OK
b) Not changed, so still reads "login" - OK?
c) OK
d) OK
e) Changed, but reads "custom block IDs are saved" (rather than "ids"?) - OK?
f) OK
#21
pre: Still reads as "// nodes keyed by uid and nid: $nodes[$uid][$nid] = $is_private;" Observe, still terminates with ";" rather than "."
a) OK
b) Now reads "* Checks whether custom node IDs are saved ..." (rather than "ids"?) - OK?
c) OK
#21 OK
#31 OK
#33 OK
#35 OK
Other than a patch for #21 pre: (terminate comment with "." rather than ";") should other points above be included?
Comment #46
jhodgdonThanks a lot for the review!!!
Let's fix #11b, #11e if it doesn't start with a verb.
On #21 the code comments - out of scope for this issue.
So we need one more small patch. Any takers?
Comment #47
bdgreen CreditAttribution: bdgreen commentedOne small patch ... ;)
Comment #48
jhodgdonOh, sorry about the confusion -- I think I misunderstood your analysis. "IDs" is correct -- that means "identifiers". "ids" is not correct. That is the plural of "id", which is a psychological term (ego, superego, id). The only things we should be fixing that are left over from previous patches would be first-line descriptions that do not start with verbs. In-code comments are out of scope.
So... we don't want to do either of the changes in this patch, and if everything else is fine, let's move on to 7.x, where we need a backport of all previous patches (or it might be easier to start over).
Comment #49
jhodgdonsorry, forgot status
Comment #50
bdgreen CreditAttribution: bdgreen commentedBackport to 7.x for docblocks (that is all "in-line code comments" have been ignored) in node.test. ;)
Comment #51
jhodgdonThis block shifts verb agreement/mood/whatever in the middle (from "create" to "verifies"):
There are several more that do that, such as:
and the next one too... I've stopped reading the patch at this point. Can you check all of the changes in the patch for that and submit a new patch please?
Hopefully 8.x does not have this problem... If it does we should go back to 8 and fix it there.
Comment #52
bdgreen CreditAttribution: bdgreen commentedRedo of backport to 7.x for docblocks ...
Comment #53
jhodgdonThanks for the new patch!
There are still issues with the patch:
a)
I think the original doc block was better for the class than the new one? It is a test class, after all.
b)
This is overly specific -- the test does a lot more than just that one thing. What was wrong with the original doc block here?
Similar problem here:
So... Again I've stopped reading this patch...
And really, let's go back to the original intent of this issue, which was "Add missing docblocks for classes and methods in node.test". Let's just have a patch that adds any documentation blocks for classes/methods/properties that are actually *missing* in node.test (though of course we do not want doc blocks added for getInfo(), setUp(), and tearDown() methods).
Thanks!
Comment #54
bdgreen CreditAttribution: bdgreen commentedThere is only one method (testSyndicateBlock) missing a docblock (excluding the getInfo(), setUp(), and tearDown() methods), plus a mere handful of properties (for which an in-line comment may have sufficed ...) - though, strictly, should these properties be included? ;-)
Comment #55
jhodgdonThanks! Yes, every property and every method on every class is supposed to have a doc block, with the exception of getInfo, setUp, and tearDown on classes currently.
For member variables, you need @var:
https://drupal.org/coding-standards/docs#var
This type of documentation also doesn't tell me what this variable stores really:
I just don't know what "For node revisions" means at all... and the other documentation blocks added for member properties suffer from the same problem. The documentation needs to document what the property is being used for, and the @var tells what data type it is.
Also, please leave a blank line between previous code and each doc block. It looks like you removed some blank lines between classes, and put the member variable doc blocks right up next to each other. That is hard to scan visually.
The indentation is also wrong here:
Comment #56
bdgreen CreditAttribution: bdgreen commentedRevised patch attached ...
Comment #57
jhodgdonThanks -- this is ***much*** better!
I found just a few things that I think could be improved... Also, when you attach the next patch, set the status to "needs review" to launch the test bot and alert human reviewers that there is a new patch. Thanks!
a)
I was kind of confused by this documentation, so I went and looked at the code... Maybe it could say something like: "The revision messages for node revisions created in the test."?
b)
Don't take out the blank line between the methods please.
c) In NodeRevisionPermissionsTestCase:
This also should be turned into a member variable doc block.
Comment #58
bdgreen CreditAttribution: bdgreen commentedChanges made as requested.
Comment #59
bdgreen CreditAttribution: bdgreen commentedClarification please on @var tag and data types. From jhodgdon link (https://drupal.org/coding-standards/docs#var) in #55 should this patch use a Namespace (for example Drupal\user\Plugin\Core\Entity) or (merely) a data type?
So, for example, in this 7.x patch we have:
Whilst (for an equivalent entry) in 8.x we have:
Comment #60
jhodgdonThis is a 7.x issue, so "@var object" is correct for 7.x -- the classes in the 8.x patch do not exist in 7.x (the 8.x docs are correct for 8.x). Thanks for asking for clarification!
So, the new patch looks good, thanks!
By the way, when you're making a large-ish patch that is just fixing a few things from the previous patch, it's nice to include an interdiff file (don't make one now, but next time):
https://drupal.org/documentation/git/interdiff
Comment #61
jhodgdonWhoops. I made another commit to node.test today on the 7.x branch (an earlier issue), and this one doesn't apply any more. Needs a reroll.
Comment #62
bdgreen CreditAttribution: bdgreen commentedRerolled ...
Comment #63
jhodgdonThanks! I'll get this committed soon.
Comment #64
jhodgdonCommitted to 7.x. Thanks to all who helped on this one!
Comment #65.0
(not verified) CreditAttribution: commentedRemoving myself from the author field so I can unfollow. --xjm
Comment #66
Andrew Answer CreditAttribution: Andrew Answer as a volunteer commented