Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Simple, after making the stuff lowercase in the module, the documentation was forgotten.
Comment | File | Size | Author |
---|---|---|---|
#30 | aggregator-docs-d7-1247982-30.patch | 2.9 KB | xjm |
#27 | 1247982_aggregator_wrong_api_doc-27.patch | 2.92 KB | kathyh |
#25 | 1247982_aggregator_wrong_api_doc-25.patch | 2.96 KB | kathyh |
#23 | 1247982_aggregator_wrong_api_doc-23.patch | 2.91 KB | kathyh |
#21 | 1247982_aggregator_wrong_api_doc-21.patch | 2.9 KB | kathyh |
Comments
Comment #1
Aron Novakonly doc, nothing else.
It affects D7 as well, the patch applies cleanly there as well. So backport is not needed, but 7.x and 8.x are both affected.
Comment #2
jhodgdonIt would be good if this was updated to follow our documentation standards too. This list of keys should be formatted as a list, following this format:
http://drupal.org/node/1354#lists
Also, in order to review this patch, I would need to verify that the documentation change is correct. The only example in core I can see where this hook is implemented doesn't even include ->items in the return value. Can you point me to where I can see this being implemented or used with lower-case array keys? Or the issue where this was changed and the API documentation wasn't? Also, if this was a Drupal 6/7 change, we need to document it on the module update page.
Comment #3
Aron NovakAbout the correctness of the change, see: http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/modules/ag...
For example in aggregator_parse_feed():
That code is called when http://api.drupal.org/api/drupal/modules--aggregator--aggregator.parser.... is invoked:
The requested improvements of the patch are done, so it's NR again.
Comment #4
Aron NovakIt was a change for Drupal 7: http://drupalcode.org/project/drupal.git/commitdiff/04662d1328bf0dbc9e04...
But before Drupal 7, the architecture of Aggregator wasn't pluggable, so it's not an update (D6->D7) issue.
Also, others are discovered this problem: http://api.drupal.org/api/drupal/modules--aggregator--aggregator.api.php...
Comment #5
jhodgdonBetter! A few suggestions:
a)
This should be
A stdClass object with the following properties:
b)
List items should be set off by : not - as in:
* - description: A string ...
And they should end in a period. Please check the list formatting guidelines on http://drupal.org/node/1354 where this is described.
c) It would be good if this said whether these strings need to be translated.
d) How is the string used as an image -- is it the filepath?
e) Actually, in general I think "A string to use for" should be left out of all of these. So it would be:
* - description: The translated, human-readable description of the field
etc.
f) Should I already know what an "entity tag header" is? I don't...
g)
Should be "The Unix timestamp when the feed was last modified" maybe?
h)
Grammatically, this is essentially a comma-splice. Should have a . instead of , after items.
i) The comments above apply to the nested list in items also.
Comment #6
Aron NovakThank you for the suggestions, we can improve the doc text further alongside fixing its the bogus caps thing :)
"translated" - i don't think we should mention it. The parser is parsing an external data file, of course the human-readable texts are in that language what the XML file provides.
Comment #7
jhodgdonActually, mentioning translated is not a bad idea. Some Drupal functions expect translated text, and a few do not. I don't think that being explicit is bad?
Looking at the current patch -- it needs proof-reading:
a)
This needs to be rewritten as an English sentence, rather than a mix of English and HTML. Also, "what" should be "that".
b) respone should be response in the next item, and can you explain what an "ETag" is? I would have no idea.
c) Please re-wrap the lines as close to 80 characters as possible without going over, everywhere in the patch.
d)
a -> the
e)
- link needs a comma after it
- tobe should be "to be"
- Does it really require both guid and link to identify if an item has already been identified? That seems wrong.
f)
I think the second sentence could be omitted? Doesn't seem to provide information.
Comment #8
Aron NovakOne quick reply:
See http://api.drupal.org/api/drupal/modules--aggregator--aggregator.process... .
It uses either guid or link. Maybe it has to be reworded with mentioning 'fallback'
Comment #9
jhodgdonRewording mentioning fallback sounds like a good idea.
Comment #10
Aron Novak"mentioning translated is not a bad idea" - for me, it would mean that the string passed somehow the drupal translation system, which is not the case. Neither we can expect that the text is in a specific language. But you can convince me, is there a place in drupal docs where we say that a text what comes from an external system or from the user, and we have no idea what language it uses and it's not processed by translation modules, and there, we say it's 'translated' human-readable text.
I updated the rest, i tried to be as precise as possible, but i can't say 100% surely that everything is grammatically and stylistically.
So anyone, feel free to improve it.
"can you explain what an "ETag" is?" - I added a link to the RFC. I think saying that it's a field of the HTTP header is really enough. If someone wants to know more, now it's possible to follow the link.
I'm not entirely sure that all the text is stylistically and grammatically correct, i just wanted to fix an error in the docs, not rewriting the whole section :), anyways, anyone is welcomed to do some further language improvements.
Comment #11
jhodgdonThe patch will not be marked RTBC (at least by me, and hopefully by anyone else either) unless it is clearly written and free of grammar issues, sorry! If you could put in the grammar fixes I pointed out in the last review, that would be helpful.
Agreed on the other points.
Comment #12
Aron NovakOne "what->that" replacement is done in the top of the previous changes (my bad, one point of your notes was skipped). This is the end of my contribution here. If nobody will improve it further, a trivial documentation error won't be fixed, it seems.
Comment #13
Aron Novakoups, marking it as NW
Comment #14
jhodgdonI am not in favor of fixing trivial documentation errors in ways that don't follow our standards, sorry! So I'm sad that you don't want to make a patch that will comply. I will go ahead an un-assign this so someone else can work on it.
Comment #15
jhodgdonAt this point, it's probably a good Novice project to redo the above patch so the doc complies with our doc standards.
Comment #16
kathyh CreditAttribution: kathyh commentedtaking a look
Comment #17
jhodgdonkathyh: thanks! The standards are at http://drupal.org/node/1354 if you haven't already figured that out. :)
Comment #19
kathyh CreditAttribution: kathyh commentedHeader documentation updated per review of above comments and node 1354
Comment #20
Aron NovakNice work!
One little thing: the etag is nothing to do with HTML, it comes from the HTTP header.
Also, under items:
"timestamp: The UNIX timestamp when the feed was last published."
This one is about the feed item, not the feed.
Comment #21
kathyh CreditAttribution: kathyh commentedThanks for the feedback - much appreciated. Attached is an update based on comment #20.
Comment #22
jhodgdonThanks! Looks pretty good... One problem though. You cannot have a paragraph break in the middle of a @param or @return, or the API module will assume that the new paragraph is part of the main function description instead of the @param/@return.
Also, the line wrapping in the list area is a bit funky. Please move words up onto previous lines if they would fit in 80 characters. And there is at least one place where there is an extra space after a . (please use just one space, although it violates everything we learned in 8th grade typing class way back when. :) ).
Comment #23
kathyh CreditAttribution: kathyh commentedUpdates per #22 and D8 /core dir change
Comment #24
jhodgdonThe formatting of this patch looks good now, thanks!
I'm not quite sure about the content.
Looking at aggregator_aggregator_parse() and aggregator_parse_feed(), I see:
- 'image' appears to be the image URL, not an HTML tag.
- There's also a 'link' field for the feed - that is not in the documentation, should it be?
- It looks like in an item, 'description' is either the full body text of the item or a summary (depending on what the feed put in there). I don't think "human-readable description" is the right documentation of that.
Comment #25
kathyh CreditAttribution: kathyh commentedUpdated per #24. Agreed with 'link' field - added.
Comment #26
jhodgdonThat's better. :)
I realized I'm still slightly uncomfortable with how the first part of this reads:
Normally, after @param $feed, we wouldn't start by saying "The $feed object...", since that is implied. And I don't think we need "The" with $feed->source_string. And... The way this is written, it's not clear that the properties described are to be added to $feed by this function, rather than expected to be part of the input $feed. So... How about rewriting this something like:
@param $feed
An object describing the resource to be parsed: $feed->source_string contains the raw feed data. The hook implementation should parse this data and add the following properties to the $feed object:
Comment #27
kathyh CreditAttribution: kathyh commentedUpdated per #26.
Comment #28
jhodgdonGreat! Thanks for all the work on this. I think we now have some actually good documentation of this hook. :)
Comment #29
catchThanks. Committed/pushed to 8.x, will need a re-roll for 7.x.
Comment #30
xjmHere's that backport. Just a reroll of documentation fixes, so this should be RTBC.
Comment #31
webchickCommitted and pushed to 7.x. Thanks!