WordPress.org

Make WordPress Core

Opened 4 weeks ago

Last modified 3 weeks ago

#47929 reviewing defect (bug)

Add missing DocBlock in `display` method of `WP_Comments_List_Table` class

Reported by: itowhid06 Owned by: dinhtungdu
Milestone: 5.3 Priority: normal
Severity: minor Version:
Component: Administration Keywords: has-patch needs-copy-review
Focuses: docs Cc:

Description

Added missing DocBlock in display method of WP_Comments_List_Table class.

Attachments (4)

47929.diff (522 bytes) - added by itowhid06 4 weeks ago.
47929_fixed.diff (519 bytes) - added by itowhid06 4 weeks ago.
47929_refreshed.diff (586 bytes) - added by itowhid06 3 weeks ago.
47929_updated.diff (595 bytes) - added by itowhid06 3 weeks ago.

Download all attachments as: .zip

Change History (16)

@itowhid06
4 weeks ago

#1 @SergeyBiryukov
4 weeks ago

  • Component changed from General to Administration
  • Focuses docs added
  • Milestone changed from Awaiting Review to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

This ticket was mentioned in Slack in #core by pento. View the logs.


3 weeks ago

#3 @noisysocks
3 weeks ago

  • Keywords needs-refresh added
  • Owner changed from SergeyBiryukov to dinhtungdu

The method description here needs to be expanded a bit. @dinhtungdu volunteered in Slack to refresh the patch.

Last edited 3 weeks ago by noisysocks (previous) (diff)

#4 follow-up: @dinhtungdu
3 weeks ago

  • Keywords needs-patch added; has-patch needs-refresh removed

@itowhid06 Thanks for the patch. As we discussed in the Slack, the description should be expanded. Consider these:

  • Adding explanation on why we need to override the display method.
  • Adding @since tag for the docs.

By the way, don't forget to add a . to the end of the comment.

#5 in reply to: ↑ 4 @itowhid06
3 weeks ago

Replying to dinhtungdu:

@itowhid06 Thanks for the patch. As we discussed in the Slack, the description should be expanded. Consider these:

  • Adding explanation on why we need to override the display method.
  • Adding @since tag for the docs.

By the way, don't forget to add a . to the end of the comment.

I really don't think there is much more to add in the description except a .. Though a lot of method DocBlock instances even some in class WP_List_Table needs to be added with a ..
And about the @since tag, I'm not sure about the version when the method was introduced.

#6 @dinhtungdu
3 weeks ago

@itowhid06 Not every class that extends WP_List_Table overrides the display method. We do it in WP_Comments_List_Table because we render a number of hidden extra comments.

Though a lot of method DocBlock instances even some in class WP_List_Table needs to be added with a ..

I agree :). But let's leave it for the other ticket.

And about the @since tag, I'm not sure about the version when the method was introduced.

I will find it for you.

Last edited 3 weeks ago by dinhtungdu (previous) (diff)

#7 @dinhtungdu
3 weeks ago

It's 3.1.0.

Actually, we did have the comment for display() before. (r17228). But it got deleted for some reason.

#8 @dinhtungdu
3 weeks ago

@itowhid06 Thanks for the patch, but I think we can be more precise for this case. What about this?

Display the comment table. Override the parent method to render extra comments.

#9 @itowhid06
3 weeks ago

@dinhtungdu , I've copied the text from display function in WP_Plugin_Install_List_Table class. If you insist then I can surely update it :)

#10 @dinhtungdu
3 weeks ago

Yeah, I still think the comment list table acts differently from other list tables, so we're better to be precise here. Please update it. Thank you so much! :)

PS: It took me 10 minutes to find out why we need extra hidden comments (They are used to append to the table when you delete or mark a comment as spam.). I think we can save other people many 10 minutes :D.

#11 @itowhid06
3 weeks ago

@dinhtungdu , updated according to your suggestion :)

#12 @dinhtungdu
3 weeks ago

  • Keywords has-patch needs-copy-review added; needs-patch removed

Thank you for your work :)

Last edited 3 weeks ago by dinhtungdu (previous) (diff)
Note: See TracTickets for help on using tickets.