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.
Add documentation comments to the views pager plugins as described in #1882558: [META] Document all views plugins types.
Comments
Comment #1
thsutton CreditAttribution: thsutton commentedAdd details to the documentation block for
PagerPluginBase
.Comment #2
thsutton CreditAttribution: thsutton commentedTagging as for parent issue.
Comment #3
dawehnerGreat!!
Ups, the same odd sentence, so yeah it should be named 'for pager plugins'.
I guess you could add the "this is displayed in the views UI" in the access issue as well.
Comment #5
samhassell CreditAttribution: samhassell commented#1: document-views-pager-base-plugin-1912650-1.patch queued for re-testing.
Comment #7
mitron CreditAttribution: mitron commentedIt would be nice if the description for the full title explained where it is used.
+ * - title: The "full" title for your pager type; the short_title is used in
+ * the views UI when specified.
Information about the short title does not belong in the title description. It should go under the short title notes.
Comment #8
babruix CreditAttribution: babruix commentedComment #9
dawehnerCool, that you are working on that.
Is type really supported by pagers?
Comment #10
babruix CreditAttribution: babruix commentedGood spot!
We haven`t found any instances of type there, so decided to remove it, attaching new patch.
Comment #11
dawehnerLast minor nitpick: this should have a dot instead of a "." at the end. Sorry.
Comment #12
babruix CreditAttribution: babruix commentedOk finally changed ";" at the line ends to "."
Comment #13
dawehnerGreat!
Comment #15
babruix CreditAttribution: babruix commented#12: drupal8.views-document-views-pager-base-plugin-1912650-12.patch queued for re-testing.
Comment #17
jthorson CreditAttribution: jthorson commentedRequeued test manually on bot.
Comment #18
dawehnerIt's just
This should be wrapped in @code and @endcode, sorry i haven't seen that before.
Comment #19
babruix CreditAttribution: babruix commentedFixed.
Comment #21
babruix CreditAttribution: babruix commentedHmm, Is something wrong with my patch or with environment?
Log says
'Installing: failed to complete installation by setting admin username/password/etc.'
which is not very helpful...Comment #22
dawehner#19: drupal8.views-document-views-pager-base-plugin-1912650-19.patch queued for re-testing.
Comment #24
dawehnerThe problem is indeed that you have "@Plugin" in there. One thing you can do is to use "@ Plugin" and mention the space in there.
Comment #25
babruix CreditAttribution: babruix commentedThanks! Corrected.
Comment #27
babruix CreditAttribution: babruix commentedAlso added space in "@ Translation" - looks it tries to import annotation if some text exists is after "@" symbol.
Comment #28
dawehnerWe don't seem to have a better solution at the moment.
Comment #29
webchickThis looks good to me, but assigning to Jennifer since it's docs-related.
Comment #30
jhodgdonUm. The first paragraph says: "...including getting setting the..." -- that can't be quite right? Is it getting, setting, or does it need an "and" in there?
In the second paragraph, I'm also wondering about the word "should". Is the @Plugin mandatory or optional? If it's mandatory, let's say "must" there. If it's optional, ... what would the alternative be? And I think the text should also explain that in your actual code, you need to remove the spaces after the @ signs.
Same concern with the "should" immediately following the example. Leave that out (maybe say "The definition uses the following keys" instead?) and put (optional) in list items that are optional. See http://drupal.org/coding-standards/docs#lists for list syntax.
And then for the "short" item, it says:
This seems to be a bit redundant? And the second (and third, if you don't remove it) lines need to be wrapped along with the first to as close to 80 characters as possible.
Comment #31
jhodgdonOh, forgot to say: I think the way you did the @code is fine and generally the docs look good -- I just think that the spaces after the @ signs need to be explained so that people know they need to remove them in their actual plugins.
Comment #32
babruix CreditAttribution: babruix commentedChanged patch following your comments.
Comment #33
jhodgdonThe first paragraph is still maybe missing an "and" before the last comma, or maybe an "etc." at the end?? It says:
Pager plugins take care of everything regarding pagers, including getting and setting the total number of items to render the pager, setting the global pager arrays.
And it should all be wrapped to 80 characters, as close as possible.
Then below the code it says:
Umm... to pass tests? I would maybe say "these are put into this sample code so that it is not recognized as annotation" or something like that? Also, you might mention it has to go at the very end of the documentation block (at least I think that is true).
Other than that, looks great!
Comment #34
babruix CreditAttribution: babruix commentedChanged all as you`ve suggested, only I`m not sure that note to remove spaces has to go at the very end of the documentation block - because for developers it will be very easy to miss it.
Comment #35
jhodgdonThis looks OK to me... if you think the note about removing spaces should go elsewhere, feel free to move it. And maybe in the last bit we should say "The plugin annotation contains these components" instead of "The definition uses the following keys"? When I read that today, I was like... what definition are we talking about here all of a sudden? It's never referred to above as a definition.
Comment #36
babruix CreditAttribution: babruix commentedComment #37
jhodgdonThat looks good to me, thanks!
I'd like someone from the Views project to review the documentation to make sure it's accurate before we commit it though (there was a review in #28 but since then the text and designation of what is optional has changed a bit).
Comment #38
dawehnerIt's a general disadvantage of the concept of plugins compared to hooks, that we had example code in the .api.php file, though we don't have a place yet for example plugins. Just wondering whether there should be an issue for that.
Beside from that the documentation look pretty good.
Comment #39
jhodgdonOn api.drupal.org, and I would think in an IDE as well, you should be able to figure out what classes in Core extend a given plugin class, and presumably those core classes would be good examples? If more is needed than that, let's start a separate issue to discuss how to provide examples and/or how to locate Core examples.
Anyway, meanwhile, the patch in #36 still needs a review for documentation accuracy.
Comment #40
dawehnerOh good point! I think this documentation is great.
Comment #41
oresh CreditAttribution: oresh commentedpatch successfully applied for latest build, no grammatical errors.
+1
Comment #42
jhodgdonOK then. :)
Comment #43
jhodgdonCommitted to 8.x. Thanks all!