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.
hook_field_attach_purge [see function field_purge_data() in modules/field/field.crud.inc]
This is part of this meta-issue: #675046: Make sure all hooks in D7 have documentation
Comment | File | Size | Author |
---|---|---|---|
#15 | 776694_15.patch | 2.09 KB | aspilicious |
#12 | 776694.patch | 2.14 KB | robeano |
#9 | 776694-02.patch | 13.83 KB | puregin |
#3 | 776694-01.patch | 13.51 KB | puregin |
#1 | 776694.patch | 2.09 KB | robeano |
Comments
Comment #1
robeano CreditAttribution: robeano commentedAs usual, I dislike my example, but it's ready for review.
Comment #2
jhodgdona)
You need to put () after function names, so the API module can turn them into links on api.drupal.org.
Also maybe in the first line -- is field_attach_purge a function name? If it isn't a function name, perhaps remove the _ characters?
b) We try to put @see at the end of the docblock. That See @link line can probably go there too.
c) The example -- maybe it should also check whether the field matches, besides just that it's node data? Though doing that might be complicated....
Comment #3
puregin CreditAttribution: puregin commentedThe attached patch does the following:
1. puts () after the function name field_purge_data()
2. moves the @see block and @link line to the bottom of the docblock.
3. Makes the initial description of each hook consistent: "Acts when... " followed by a description from the
one-line description of the invoking function. This avoids the ambiguity of "Act on X" (i.e., does it act when
X occurs, or does it perform an action on X).
Comment #4
puregin CreditAttribution: puregin commentedComment #5
puregin CreditAttribution: puregin commentedGah. I think I have status right now...
Comment #6
aspilicious CreditAttribution: aspilicious commentedIf I understand dreditor, this sentence exceeds 80 characters, that isn't allowed.
This is a hook, so acts have to be act.
http://drupal.org/node/1354#hooks
Same as first one + same thing about the acts
Trailing whitespace
I don't know who made dreditor script but he/she made me happy :D. It would be awsome if there was an optimised script for chrome to ;). I can install it on chrome but it doesn't do &!@#
98 critical left. Go review some!
Comment #7
aspilicious CreditAttribution: aspilicious commentedComment #8
puregin CreditAttribution: puregin commented@aspilious - thanks, dreditor sounds cool, I will check it out.
I've fixed the longer comment lines that I added/modified.
I notice that hooks currently don't seem to follow a clearly established convention for summary line documentation. I'm looking for example at the JavaDocs standard:
I prefer this, but I see examples of both conventions in our documentation (i.e., both 3rd person declarative and 2nd person imperative)
I think I've got the diff current, it the correct (root) directory now. Sorry about the flailing.
Comment #9
puregin CreditAttribution: puregin commentedComment #10
jhodgdonPlease don't make patches that go beyond the scope of the issue. If you feel that a lot of things need changing, either file one big issue saying "this needs changing" or a bunch of little ones. And please search the issue queue to see if the issue has been reported first.
So all of the patches from #3 on are not relevant to this issue. robeano is at this moment going back to the patch in #1 and fixing it to address the relevant comments.
Also, as a general courtesy, when an issue is assigned to someone, it is polite to ask them before taking it over for yourself. Thanks!
Also, just looking at your latest patch, some of the changes are good (such as informations -> information), but others are not. For instance,
Please check http://drupal.org/node/1354#hooks
We want this to start with "Act" not "Acts", and the short description should be one line if at all possible.
Thanks!
Comment #11
puregin CreditAttribution: puregin commented@jhodgdon, thanks for the feedback. I tend to try to fix all instances of a given issue for a particular file at once, but I see your point.
Regarding 'Acts' vs 'Act', I was taking my guidance from #487802: Function doc standard for Drupal is non-standard in industry, which seems to indicate that 3rd person declarative is preferred.
And sorry @robeano if overstepped the bounds of helpfulness :)
Comment #12
robeano CreditAttribution: robeano commentedNew patch which follows jhodgdon's advice given in #776694-2: hook_field_attach_purge is not documented
* Added ()
* field_attach_purge was wrong in the one-line description
* Moved the @see to the bottom of the block
puregin, thanks. I was more confused than offended. Ready for another review.
Comment #13
jhodgdonpuregin: that is not how we do things in the Drupal project. Google "killing kittens" and see what you can find.
And please read the standards page before you comment on whether the doc is following standards. Thanks!
Comment #14
blg@bgreenaway.com CreditAttribution: blg@bgreenaway.com commentedHi robeano, sitting here with jhodgdon at DrupalconSF doc sprint and spotted one thing, suggesting on jhodgdon's advice another.
First, the patch has two additional lines than it needs after '* @see field_purge_data()', one is an empty comment line, another appears to be a blank and empty line. (see below)
+ *
+ * See also @link field_purge Field API bulk data deletion @endlink
+ * @see field_purge_data()
+ *
+
+ */
And we're pretty sure that the line 'See also @link' would benefit from being changed to '@see @link'.
Everything else AOK, plz re-run test (not sure I can at this stage, but will try later if you don't beat me to it:)
Comment #15
aspilicious CreditAttribution: aspilicious commentedrerroll
Comment #16
aspilicious CreditAttribution: aspilicious commentedComment #17
aspilicious CreditAttribution: aspilicious commentedFixed the only issue pointed out in #14
Comment #18
jhodgdonagreed, looks good!
Comment #19
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.