Part of meta-issue #1310084: [meta] API documentation cleanup sprint.

CommentFileSizeAuthor
#7 1807612-7-ban-docs.patch1.83 KBAlbert Volkman
#1 1807612-1-ban-docs.patch2.42 KBLars Toomre
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lars Toomre’s picture

Lars Toomre CreditAttribution: Lars Toomre commented
FileSize
1807612-1-ban-docs.patch2.42 KB

Here is a small patch (untested locally) that cleans up a few items in the documentation of the new Ban module.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Is this patch ready for review, or is it just a partial/not quite ready patch?

Lars Toomre’s picture

Lars Toomre CreditAttribution: Lars Toomre commented

To be clear, @jhodgdon I am frankly frustrated with doing a full review of a core module and a subsequent review getting hung up on a small quibble issues (like what happened with the Menu module review).

This module is non conformant with documentation standards. The patch in #1 identifies at least once instance why. There are likely other documentation standard violations in this module.

I have not bothered to explore further, especially given my experience with the full reviews of the Action, Aggregator, Locale, Menu, Node, Simpletest, System and User modules. Others are welcome to move this module closer to D8 compliance.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

RE #2 - the status of this issue is "active". You didn't change the status to "needs review" when you uploaded the patch, so I was just asking if you forgot to change the status, or if the patch was waiting for more to be done. I have not looked at the patch and was not commenting on it or on this issue in any way.

Lars Toomre’s picture

Lars Toomre CreditAttribution: Lars Toomre commented
Status: Active » Needs review

Let's test this patch in #1 since it seems to be correct upon further review.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

Thanks!

Let's get rid of this change:

- * @see ban_ip_form_validate()
  * @see ban_ip_form_submit()
+ * @see ban_ip_form_validate()

The previous order was more logical.

Other than that, looks good!

Albert Volkman’s picture

Albert Volkman CreditAttribution: Albert Volkman commented
Status: Needs work » Needs review
FileSize
1807612-7-ban-docs.patch1.83 KB

Re-roll of #1 with suggestion from #6 applied.

cosmicdreams’s picture

cosmicdreams CreditAttribution: cosmicdreams commented

found another small fix in BanSubscriber.php. Will fix tonight.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Issue summary: View changes
Status: Needs review » Closed (won't fix)

These issues are a lot of work with very little tangible payoff, so I'm closing the rest of them as "won't fix". Your efforts on working on this issue were appreciated... it was just my fault for starting a task that was very difficult to get right.

Let's instead put our effort into fixing and reviewing documentation that is really unclear and/or wrong, and I hope that the people who worked on these issues are not afraid to jump into a more reasonable issue!