Posted by agentrickard on December 12, 2011 at 8:43pm
10 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | documentation |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | docs-cleanup-2011, needs backport to D7, Novice |
Issue Summary
Part of meta-issue #1310084: [meta] API documentation cleanup sprint
Comments
#1
And the patch, which clarifies the constants defined by the module. All else is fine.
#2
The last submitted patch, 1368872-syslog-api-cleanup.patch, failed testing.
#3
#1: 1368872-syslog-api-cleanup.patch queued for re-testing.
#4
Huh. Will that actually work in the API module as documentation for that DEFINE? I kind of doubt it. We should test that before committing this patch. Probably the docblock would need to be inside the if() statement... I can test this later.
Also, when linking to function docs on php.net, we try to use a non-language URL (/en/ should not be in the URL). Check other examples in Drupal core.
Amazing that the syslog module wouldn't need any other fixes!
#5
So do you want the IF test checked or are we passing on that until later?
#6
Um... I am not sure what you're asking? I'm just saying that the doc block for the define() needs to be, I believe, directly before the define() line, not before the if() block.
#7
OK. Got it. But does it need to go before both instances or just the first one?
#8
Ah [finally looked at the code, not just the patch].
I guess both of them should have docblocks, within the if/else respectively. I guess we should also still see if the API module will recognize them inside or outside the ifs.
#9
I suspect API module will choke, since its the same variable each time.
#10
Yes, you're right. The API module allows for multiple "objects" (functions, defines, classes, etc.) to have the same name within the same project, but only one per file (the URL is composed of project / file / object name).
So let's just document it on one (inside the if() I think?), but document what it does for Windows and non-Windows in the same docblock.
#11
Revised.
#12
That looks quite reasonable to me, thanks!
#13
Looks good to me, committed/pushed to 8.x. I think we could backport this.
#14
D7 patch.
#15
Same patch; definitely needs port. Thanks!
#16
YIKES. The commit to 8.x accidentally included the patch from #375397: Make Node module optional but we're handling the rollback in that issue.
#17
This will need to be reapplied for D8 (see #375397-125: Make Node module optional). ;)
#18
Commit of #11 had to be reverted:
http://drupalcode.org/project/drupal.git/commit/256a10dd192cab26c5c919b4...
One more time, please! :)
#19
Was reverted in 8.x. Needs to have ONLY the patch in #11 committed to 8.x.
#20
Let's try that again. Committed/pushed to 8.x.
#21
Committed and pushed to 7.x. Thanks!
#22
Yay, no sneak API changes this time! ;)
Reopening for some minor followup--
syslog_facility_list()has a wrong verb tense, as doesSyslogTestCase::testSettings(), andSyslogTestCaseis missing a docblock entirely.#23
I don't think agentrickard has worked on this for a while, so unassigning. Also tagging.
#24
Addresses #22. Changed verb tense and added a docblock to
SyslogTestCase#25
Thanks @kid_icarus! That looks good. One minor thing:
+++ b/core/modules/syslog/syslog.testundefined@@ -5,6 +5,9 @@
+ * Functional tests for syslog.
This summary should start with a verb as well.
#26
The summary now starts with a verb :) I chose "Represents", as I think that's an accurate verb for a class description.
+++ b/core/modules/syslog/syslog.test@@ -6,7 +6,7 @@
- * Functional tests for syslog.
+ * Represents functional tests for syslog.
#27
#28
Hm. I would agree with you in Java, but I'm not sure this class really "represents" anything -- it's just a collection of test functions.
Perhaps following the model on other test classes in Drupal Core would be more sensible? Such as:
/*** Tests the Color module functionality.
*/
#29
And otherwise, this patch looks fine! I checked and nothing else but these few changes needs to have docs fixes. Of course, the module only has about 25 functions all together. :)
#30
Yeah, you're right on the class not representing anything :) I was influenced by http://drupal.org/node/1354#classes in that decision.
+++ b/core/modules/syslog/syslog.test@@ -6,7 +6,7 @@
/**
- * Represents functional tests for syslog.
+ * Tests the Syslog module functionalality.
*/
#31
#32
D'oh, I misspelled functionality as "functionalality".
+++ b/core/modules/syslog/syslog.test@@ -6,7 +6,7 @@
/**
- * Tests the Syslog module functionalality.
+ * Tests the Syslog module functionality.
*/
#33
Thanks! Patch in #32 committed to 8.x and 7.x. I think we are officially done cleaning up syslog. :)
#34
Automatically closed -- issue fixed for 2 weeks with no activity.