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.
Problem/Motivation
Part of meta-issue #1310084: [meta] API documentation cleanup sprint.
Proposed resolution
Correct the following in includes
files beginning with A-C:
- Ensure that all
@return
lines are preceded by a blank line. - Ensure that
@see
and@ingroup
lines are always at the end of docblocks. - Ensure that all lines in doxygen blocks are 80 characters or fewer (excluding the terminal newline).
- Ensure that all functions, classes, interfaces, and methods have one-line summaries that are clear, use the correct verb tense, and follow specific standards in http://drupal.org/node/1354.
- Make incidental style and grammar corrections only to those docblocks already being updated.
Remaining tasks
Patch in #43 includes the requested cleanup in all files in includes/
beginning with A through C. The patch incorporates numerous corrections but needs to be reviewed.
Followup tasks for other patches
- WTFs @
format_date()
WRT_format_date_callback()
usage. - Check list indentations.
- Check for double spaces after periods.
Followup issues
- #1326456: Add missing @param in includes A-C, plus other corrections to some docblocks
- #1326458: Add missing @return in includes A-C
- #1326452: Clean up documentation of optional parameters in includes A-C
- #1326472: Add capitalization and periods to several inline comments in common.inc.
- #1326482: Clean up minor code style issues in archiver.inc
- #1326574: Correct list stuff in docs for includes/database
Related issues
Comment | File | Size | Author |
---|---|---|---|
#100 | manual-diff.txt | 53.47 KB | xjm |
#99 | d7-a-c.patch | 116.22 KB | xjm |
#99 | manual-diff.txt | 47.63 KB | xjm |
#94 | a-c-91.patch | 127.24 KB | xjm |
#94 | interdiff-90-91.txt | 467 bytes | xjm |
Comments
Comment #1
xjmDurr.
Comment #1.0
xjmAdding working notes to summary.
Comment #1.1
xjmUpdated issue summary.
Comment #1.2
xjmUpdated issue summary.
Comment #1.3
xjmUpdated issue summary.
Comment #1.4
xjmUpdated issue summary.
Comment #1.5
xjmUpdated issue summary.
Comment #1.6
xjmUpdated issue summary.
Comment #1.7
xjm.
Comment #1.8
xjmUpdated issue summary.
Comment #1.9
xjmUpdated issue summary.
Comment #1.10
xjmUpdated issue summary.
Comment #1.11
xjm.
Comment #2
jhodgdonSaw your summary...
Regarding callbacks, I just noticed that also and filed:
#1315992: No standard for documenting menu callbacks
so we could create a standard.
Regarding classes/interfaces, we do have standards:
http://drupal.org/node/1354#classes
And I think you can clean out extra lines of whitespace at tops of files in this patch -- that doesn't add in any significant way to the review burden.
Comment #2.0
jhodgdonUpdated issue summary.
Comment #2.1
xjmUpdated issue summary.
Comment #3
jhodgdonRegarding helper/shutdown functions: I say leave them as-is for now... I'm not too concerned about 1-line docs that start with:
Something blah: verbs in the right format.
Comment #3.0
jhodgdonUpdated issue summary.
Comment #3.1
xjmUpdated issue summary.
Comment #3.2
xjmUpdated issue summary.
Comment #3.3
xjmUpdated issue summary.
Comment #3.4
xjmupdated summary
Comment #3.5
xjm.
Comment #4
xjmWork so far, through
bootstrap.inc
, minus the unfinished tasks listed above.Comment #5
xjmWhitespace.
Comment #5.0
xjmUpdated issue summary.
Comment #6
xjmDidn't finish fixing the wrapping here.
Do we want the word "function" here?
Looks like this needs wrapping fixed.
Should be a docblock.
how i can computer? why is blue? views? modules? wtf?
Comment #6.0
xjmUpdated issue summary.
Comment #6.1
xjmUpdated issue summary.
Comment #6.2
xjmUpdated issue summary.
Comment #6.3
xjmUpdated issue summary.
Comment #6.4
xjmUpdated issue summary.
Comment #6.5
xjmUpdated issue summary.
Comment #7
xjmAttached includes the above cleanups and is complete through
bootstrap.inc
, except for the points mentioned in the summary (#1315992: No standard for documenting menu callbacks for the ajax menu callbacks and the point about overridden methods). 1563 lines already. :)Comment #7.0
xjmUpdated issue summary.
Comment #7.1
xjmUpdated issue summary.
Comment #7.2
xjmUpdated issue summary.
Comment #7.3
xjm.
Comment #7.4
xjmUpdated issue summary.
Comment #7.5
xjmUpdated issue summary.
Comment #7.6
xjm.
Comment #8
xjmThrough common.inc. Haven't proofread it yet. Outstanding issue with
JS_LIBRARY
docblock.Comment #8.0
xjmUpdated issue summary.
Comment #9
xjmThat patch is pretty big already and there are still 16 files to go. I am wondering if it might be better to split off these files and do the rest in a second patch.
Comment #9.0
xjmUpdated issue summary.
Comment #9.1
xjmUpdated issue summary.
Comment #10
xjmI'll update the meta issue summary for this.
Comment #11
xjmOnly one path, so one line.
Capitalize.
Add an @see instead and change to "Formats the percent completion for a batch set."
Capitalize and add @see.
Whitespace.
I vaguely remember something about not using @todo; check docs.
Add an @see to drupal_http_request().
Not sure if dropping the word block is advised since that would make it different from all the other constants in the group. Try something else to get under 80 char.
Add "Callback: " prefix?
1. Add "Callback: "?
2. Whitespace
Change to "Pre-render callback: Adds the elements needed..."
Capitalize.
Pre-render callback: Renders #browsers...
Pre-render callback: Renders a link...
Pre-render callback: Collects...
Pre-render callback: Appends...
Sorting callback: Sorts...
Sorting callback: Sorts...
Sorting callback: Sorts...
Sorting callback: Sorts...
Also
drupal_sort_css_js()
.how i can computer? why is blue? views? modules? wtf?
Comment #12
xjmI'll wait on these corrections until we RTBC #1315992: No standard for documenting menu callbacks.
Comment #13
jhodgdonYou reviewed your own patch? :)
Comment #14
xjmIt's still marked NW. :P
Comment #14.0
xjmUpdated issue summary.
Comment #14.1
xjmUpdated issue summary.
Comment #15
xjmUntagging.
Comment #16
xjmComment #17
xjmSee if "short form" documentation should be used. (For the second for sure.)
Fail.
Missing *.
Switch to callback pattern.
-23 days to next Drupal core point release.
Comment #18
xjmComment #19
xjmAlright, I think this is ready for another pair of eyes. :)
Comment #19.0
xjmUpdated issue summary.
Comment #20
xjmJust noticed a missing period in #18 -- reroll incoming.
Comment #21
xjmMissing period for
drupal_sort_css_js()
summary fixed. Also rerolled against current 8.x.Comment #22
xjmComment #23
jhodgdonLooks pretty close! A couple of things I noted:
a)
The rewrapping here inserted a * in the line. Oops!
b)
A one-liner should be enough here. The param/return docs should go on SystemQueue::claimItem() (and yes, I'm aware that function doesn't currently have any doc, but hopefully by the end of this process it will).
c) Same applies to other overrides in this file.
d)
I think this should be implements rather than overrides? This is a class that is implementing an interface, not a class that is extending a base class and overriding some of its methods. Same applies to several other methods in the patch.
e)
Might need a "the" in there? Other "the" spots:
+ * Sets up script environment and loads settings.php.
+ * Attempts to serve a page from cache.
+ * Initializes database system and registers autoload functions.
etc.
f) Maybe not the right issue to clean this up, but this is not accurate:
The bootstrap phases are integer constants, not strings. Separate issue?
g)
Extra space between * and Parses.
h)
Just a reminder that we're working on the standards for this on #1317826: Provide general documentation standard for callbacks (xjm knows this :) ).
i)
Our standard for optional is lower-case, so deprecated should be too?
j)
One space after ".".
k)
guarantee -> guarantees
l) Some of the rewrapping in the patch is good, but some seems to be a bit over-agressive. Example:
By my editor's count, the original line (in the file, not in the patch file) was 80 characters, so this didn't need to be rewrapped. There are quite a few others in the patch that I think also didn't need to be rewrapped -- you might want to check your editor settings. Note that patch files have an extra character at the beginning of the line, so don't count that.
Phew! Big patch!!
Comment #24
jhodgdonThe review above was for the patch in #18.
xjm: when you un-assign the issue after uploading a patch, that indicates to me that you plan to not work on it any more if it needs work... is that the case? Leaving the cross-posted assign in hopes that is not the case. :)
Comment #25
xjmEr, sorry, I just meant to indicate I wasn't actively working on it at that time. :)
For the claimItem() methods, they override the default value that the base class (edit: Er, interface? damn OO) method provides. So I think the @param is needed there.
Re: The line wrapping -- The script I ran listed all the lines were greater than 80 characters. In my editor, there were a lot of these same lines that wrapped exactly one character in an 80-column terminal. I wonder if the script (and my editor) are counting the linebreak character in the total, and if that is desirable or not?
Comment #26
xjmHmm, I will try to roll a patch without the char-limit hunks for now 'til I can figure out the deal there.
Comment #27
xjmUploading for safekeeping a patch that just backs out all rewrapping for now (except for obvious things that were way over). #23 is not addressed yet (except those points relevant to the backed out hunks, naturally). Thank you, git gui. And, hey, it's 1/3 smaller this way!
Comment #28
xjmShould really do this as well. I'll double check this tomorrow.
Comment #29
xjmOr now.
Yeah, add that back.
This one too.
And him.
Comment #30
xjmHere's a printout of the lines I see as over 80 chars (the top key indicates line length). @jhodgdon, could you confirm whether the lines listed as 81 appear to be the correct (maximum) length to you?
Comment #31
xjmAlright, addressing #23.
a: Reverted in line wrap revert.
b, c: The default value for
$lease_time
is different from that inSystemQueue::claimItem()
, so I think the@param
lines need to be added there.d: I'm a bit OO-challenged and consequently foggy on the terminology here so bear with me. Can you confirm if this is correct:
class Foo extends Bar
Then: The short-form docblocks for methods can begin with "Overrides"
class Foo implements Bar
Then: The short-form docblock for methods can begin with "Implements."
class Foo extends Bar implements Baz
Then: If the method is only found in Baz, "Implements." If the method is found in Bar, "Overrides."
e: I guess "the" seems optional to me in those, but I will fix them with your suggestions.
f: I hate that docblock and I am already changing it, so I will kill that kitten.
g: Ah, thanks, will fix that.
h: Will go through and fix these to what we settled on.
i: I was deferring cleanup of the optional parameter stuff to a separate issue since this patch is so long.
j: Thanks. When I was 11 and I took typing, the teacher taught me to put two spaces after periods and I do it without thinking. I am mad at that teacher. :)
k: Thanks, I'll fix this.
l: Covered in #25 and #30.
Comment #32
jhodgdonRE #30 line length - the lines you have marked as 81 characters do not wrap in my editor. Do they in yours? Maybe we need to get a few opinions on this. In Emacs, when I set the fill-column to 80 characters and auto-wrap those lines, they stay as-is. (ctrl-u 80 esc-x set-fill-column, esc-q)
On the other hand, for instance in actions.inc, on line 80 or so, there is a @param with a paragraph that could be wrapped longer. Is your script doing that too, or just catching the over-length lines?
RE #31 -
b/c - ok
d - yes you are correct
i - all I meant is that if (deprecated) is there, the word "deprecated" should be lower-case.
j - Me too, two spaces is a hard habit to break.
Comment #32.0
jhodgdon.
Comment #32.1
xjmUpdated issue summary.
Comment #33
xjmAttached addresses the above stuff, including re-wrapping for paragraphs with lines over 80 when the linebreak is excluded. Haven't proofread yet.
Edit: We dropped 50k by not rewrapping the 81 char lines. :)
Comment #34
xjmWhitespace.
Typo (missing space in "Callbackfor").
Also, I'm on the fence as to whether it's worthwhile to include a list when there's this many.
Fix wrapping here while we're at it.
Comment #35
xjmCheck wrapping here; that looks like a big gap.
Change to "given its hash."
Check wrapping.
The word "the" is not needed here.
The parameter default is overriden, but not the return value, so remove @return.
With further kitten peril, this sentence could be simplified to "To access the Drupal database from a script without loading anything else, include bootstrap.inc and call..."
Check wrapping.
Should this be "defines"?
Check wrapping.
From THE cache, to be consistent with other methods?
Adjust wrapping?
Unnecessary wrapping?
Looks like list indentation might be incorrect; maybe we can fix that.
Check wrapping.
Capitalize HEAD tag for consistency.
AN access denied error.
Missing @param. Also, no colon after @see.
Missing @param and @return.
Might want to change this to "adds a '/' to the beginning and end of the returned path..." It's a bit confusing as written.
Whitespace.
Fix rewrapping.
Whitespace.
Should this be in an @see at the end?
From THE cache.
Fix wrapping.
AN SQL query. Edit: I guess this depends whether or not one reads it as "Sequel."
Fix wrapping.
Comment #36
xjmOops, one last bit:
Assembles THE Drupal FileTransfer registry.
So it looks like I missed reverting a few of the 81 char rewraps before, but now I think that should be all of them.
Comment #37
xjmSomebody killed a different kitten so attached tries to resolve a merge conflict. Sending to testbot to make sure it isn't brokened.
Comment #38
xjmBack to NW now that testbot is queued.
Comment #39
xjmSomething weird happened here.
Just use "Overrides DrupalCacheArray::resolveCacheMiss()."
An HTTP header.
Can drop the word "Returns."
-26 days to next Drupal core point release.
Comment #40
xjmAlright, your turn again. :)
Comment #41
xjmArgh. Should have been a period. Fix attached.
Comment #42
Lars Toomre CreditAttribution: Lars Toomre commentedThanks for working on this issue xjm. Cleaning up the documentation in the a-c includes files is producing a big patch that takes a while to review. In reviewing this patch, I noticed that there are inconsistencies in how list items are formatted.
For instance, drupal_add_js() uses the pattern "- key:", while drupal_render_page() uses the pattern "- 'key':" (single quotes around the key). I tend to use the latter pattern in my own custom code. However, reviewing Doxygen and comment formatting conventions for list formatting just now, I see that "- key:" is the Drupal documentation standard. Hence, this patch needs work to correct the key lists in at least the following functions:
format_plural()
url()
drupal_render_page()
drupal_parse_dependency()
I did not check the source files themselves. Hence, there may also be other functions that need to be corrected for improper list item formats that are not included in the #41 patch file.
Comment #43
xjmNoticed a few things I'd originally fixed in #18 that were backed out with the wrapping in #27. Attached:
I've also attached an interdiff against the patch that was previously reviewed in #18.
Comment #44
xjmThanks for reviewing! However, things like this are being deferred to followup issues. I'll add it to the issue summary.
Edit: Also, we are not backporting the cleanups to D7, per webchick and jhodgdon, because they are too disruptive.
Comment #44.0
xjmUpdated issue summary.
Comment #44.1
xjmUpdated issue summary.
Comment #45
xjmJust to reiterate: I personally am not going to work on cleaning up everything in all the doxygen in the includes directory in a single patch. Otherwise, this beast will be stuck in reroll hell forever. :) Please open followup issues for additional fixes that aren't part of the sprint once these patches land.
Also, this will need to be rerolled after Tuesday. I'll keep fixing this one and try to merge it, but I will start afresh after Tuesday for the rest of the includes directory, based on what we've worked out over the course of this issue.
Comment #45.0
xjmUpdated issue summary.
Comment #45.1
xjmUpdated issue summary.
Comment #45.2
xjmUpdated issue summary.
Comment #45.3
xjmUpdated issue summary.
Comment #45.4
xjmUpdated issue summary.
Comment #45.5
xjmUpdated issue summary.
Comment #45.6
xjmUpdated issue summary.
Comment #46
Lars Toomre CreditAttribution: Lars Toomre commentedRegarding #44 about list formatting, I have proactively created a follow-up issue #1326574: Correct list stuff in docs for includes/database with an attached patch. That patch contains a correction for the malformed list in the common.inc as well as approximately 35 other core files where list formatting does not conform to core documentation standards.
Depending upon which patch gets committed first, this patch may need to be re-rolled. I am happy to re-roll mine to keep up as the documentation sprint patches are committed.
Comment #46.0
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #47
jhodgdonLet's keep the list formatting in a separate issue. Thanks Lars!
Comment #48
xjmRerolled for
core/
directory.Comment #48.0
xjmUpdated issue summary.
Comment #49
jhodgdonLooks pretty good... A few notes/questions/todos:
a)
Does this need a comma before "given"?
b)
"handler to handle" seems a bit weird. How about "Form element processing handler for the #ajax..."?
c) function authorize_filetransfer_form() -- needs @see for its validate/submit handler functions. Validate and submit should also point to each other with @see.
d)
Two spaces after the period - should only be one. Please check the rest of the file for this if it's easy in your editor. This exact line with two spaces appears at least twice.
Whew, long patch! That's all I see that needs help...
Comment #50
xjmOkay, I actually wrote a shell script last week to parse and check for this, so I don't know how it got by me! Edit: Maybe I only checked common.inc.... hmm.
I'll reroll with these fixes shortly.
Comment #51
xjmCorrected the above and found a couple more superfluous post-period spaces in files other than common.inc.
Comment #52
xjmComment #54
jhodgdon#51: a-c-51.patch queued for re-testing.
Comment #55
Lars Toomre CreditAttribution: Lars Toomre commentedHi @xjm and @jhodgdon. The power is still out for much of Connecticut including me. Hence, I only am getting very intermittent Internet access like now as I pick-up dinner.
I have been working off-line (via a car generator) on the list items documentation issue (and its associated (optional)/(required) descriptions) #1326574: Correct list stuff in docs for includes/database. That issue has now expanded to almost 70 affected core files and will be a very big patch if it all goes in at once.
My current thinking is that #1326574: Correct list stuff in docs for includes/database probably will need to become a meta issue with several sub-issues of its own to get it all committed. (I need to roll a new "big" patch for that issue now that I have just now pulled down the current Drupal version including the movement to /core.)
Three of the files from that issue (archiver.inc, bootstrap.inc and common.inc) overlap with this issue. Hence, I am now thinking that I will try to add your patch (as is if too had been committed) and then overlay my changes to those three files on top to see what the results look like. If I can get git to behave sufficiently, I will then try to roll a smaller patch with only the three overlapping files for further discussion starting tomorrow (hoprfully!!!).
This make sense? I will try to check back later in the evening for any updates or thoughts. More to come ...
Comment #57
xjmOkay, finally testbot doesn't hate me anymore.
Comment #58
Lars Toomre CreditAttribution: Lars Toomre commentedRe-testing patch in #51. I have tried to apply it in order to then roll lists changes patch, but I am getting 'patch failed: core/includes/common.inc:70'. Let's see the test bots think ...
Comment #59
Lars Toomre CreditAttribution: Lars Toomre commented#51: a-c-51.patch queued for re-testing.
Comment #61
xjmJust needs to be rerolled again. You don't need to run testbot to guess that. ;)
Comment #62
Lars Toomre CreditAttribution: Lars Toomre commented@xjm: Given my minimal Git skills, I was presuming that I had done something wrong while setting up branches, etc.
Comment #63
xjmHere's the needed reroll. I've also attached a text file showing the changes made to resolve the merge conflict (which are the only differences betweeen this and 51).
Comment #64
Lars Toomre CreditAttribution: Lars Toomre commentedThe 'easy lists' patch to wrap around this one when it is committed is now largely done. That patch can be found at #1333534-1: Further cleanup for documentation in core/includes files starting with A-G . It may need to be re-rolled for this issue and/or #1317620: Clean up API docs for includes directory, files starting with D-G get committed.
However, I am glad to have much of the heavy lifting from my mega patch separation now done. I can re-roll the pieces whenever needed so it all gets committed. Let me know how I can further help you @xjm.
Cheers!
Comment #65
xjmThis patch needs to make it to RTBC before anything else. :)
Comment #66
jhodgdonLooks good to me, thanks!
Comment #67
chx CreditAttribution: chx commentedUnbelievable work.
Comment #68
xjmRerolling after #1006714: drupal_get_path doesn't work for profiles.
Comment #70
catchOK overall this looks great, except I got to here which looks a bit strange:
Two things:
- the explanation of the param is identical.
- xjm explained in irc that the default value of the paramater is changed from 30 in the base class to 0 here, however while it mentioned that the default is 0, it doesn't mention that this is what has been changed.
Otherwise looks great! Didn't get all the way to the end of the patch yet though.
Comment #71
xjmNote that these two classes are not actually extending the same class. One is extending
SystemQueue
and the other is extendingMemoryQueue
. I don't believe the method exists in the base queue... thinger.What I'll do is:
Comment #72
catchWould be good to explain why we're overriding as well. This is in the @file description so could maybe move from there?
Comment #73
jhodgdonIs the only difference between the overridden method and the base method the different default for the parameter? If so, I suggest:
---
Overrides MemoryQueue::claimItem().
The only difference between this function and MemoryQueue::claimItem() is that the $lease_time parameter defaults to 0 instead of 30.
---
I think we don't need to include @param here, since the use of the parameter is documented on the base class.
If there are other differences, they can be documented in that sentence instead of "the only difference".
Regarding #72:
I think the bit about what the class as a whole does belongs in the class's docblock, not the claimItem() docblock, if it isn't there already.
Comment #74
jhodgdonHere are the standards for class/method docs:
http://drupal.org/node/1354#classes
It doesn't specifically say what to do in this case...
Comment #75
xjmAnd here's the original issue (which @jhodgdon found for me in IRC): #718596: Lacking standards for documenting classes, interfaces, methods
Comment #76
xjmHm, thought about it a bit and I think it's misleading to say "the only difference"--the methods differ functionally. The word "unlike" seems promising, though.
I do think catch has a point about documenting what the differing default means for the use of the method. I'll see what I can come up with.
Comment #77
xjmLet's start with this.
Comment #78
xjmJust noticed that this should be "encodes it" rather than "encodes them." We're encoding the URI, not dangerous protocols. ;)
Comment #79
xjmNeeds a reroll for #1227942: Rename format_username() now that it's in user.module.
format_username()
got moved into the user module, so the changes to that function didn't apply anymore. They were in hunks by themselves, so just rerolled without them. (For real this time.) :)Comment #80
xjmRerolled for #1077878: Add HTML5shiv to core.
Comment #81
xjmThe interdiff in #77 has the changes that currently need to be reviewed.
Comment #82
jhodgdonSorry for the delay on reviewing this (large) patch...
I reviewed this patch through bootstrap.inc. Found the following:
a)
Two spaces after . Occurs a few times in this file.
Actually that's it... If someone wants to start reviewing at
diff --git a/core/includes/cache-install.inc b/core/includes/cache-install.inc
that would be cool! Or I'll get back to it when I can...
Comment #83
xjmRe: #82: Ah yes, in the sentences I added, visible in the interdiff in #77. :(
The patch after
batch.queue.inc
is the same as #68, aside from the changes in #78-#79 .Comment #84
xjmAttached fixes the double spaces after periods in
batch.queue.inc
, plus one other inarchiver.inc
. I've also attached a manual diff against the previously RTBC patch in #68 to illustrate what has changed since then.Comment #85
xjmAs the diff above shows, these are the only differences between the patches after batch.queue.inc. The first is a correction, and the other removes changes to a moved function.
Comment #86
jhodgdonI reviewed the rest of this patch, starting with cache-interface.inc. Found a few things to fix:
a)
There should not be a blank line between the params (just above $bin is another @param too)... And while you're at it, "bin $bin" should be "cache bin" (without the redundant, in text, $bin). Also the $wildcard one could use a bit of fixing, like it could just say "If TRUE, cache ..." rather than "If $wildcard is TRUE...".
b) @param for $key is missing -- maybe that's OK, but since $item is there, it seems a bit odd:
c) A bit of a nitpick, but the verb tenses in the list items could be made parallel:
Insert/Escape or Inserted/Escaped... probably the latter, to match the tense of the sentence before the list?
d) List formatting (missing :):
e)
id -> ID (sorry, not your fault, just a pet peeve of mine...)
f)
Line wrapping is a bit odd here? I don't think the | needs to be all on one line?
Also, punctuation: "constants; e.g., DRUP..."
g) One sentence for one-line function summary:
If you make a new patch, and interdiff between the new patch and #84 would be helpful. Thanks!
Comment #87
xjmHmm, I thought we'd decided earlier that we weren't going to do any list reformatting or other cleanups for rewrapped lines in this patch because it was so big already (and there was another issue targeting the list reformatting).
It will need to be rerolled anyway because of the const/define patch, though. I was hoping to get this in before that one since it could be automatically generated and this can't. Oh well...
Edit:
Ah, I thought it did, because when I wrapped it in the middle of the expression it wasn't clear that it was a bitwise or. Maybe it should be inside
@code
?Comment #88
jhodgdonRE #87...
List formatting -- I think if there is a lot of list formatting, that would be bad to include, but in these cases you have already done some cleanup on the list formatting (like fixing indenting). It just didn't quite get cleaned up all the way (like adding colons). I would avoid large swaths of list formatting, which would greatly add to the size of the patch, but if you are cleaning up a few lines that is OK.
OK on the odd wrapping. Putting it in @code does make sense.
Comment #89
xjmAlright, thankfully rerolling for the
const
change only required a simple rebase, thanks to git. Adding the fixes from #86 now.Comment #90
xjm$key
parameter for botharray_walk()
callbacks I found.format_string()
andt()
, rather than duplicating those functions' parameter descriptions. I also added@see
toformat_string()
andt()
at the end since those functions also help explain howformat_plural()
works.Comment #91
Lars Toomre CreditAttribution: Lars Toomre commentedI have not re-rolled the list formatting patch based on @xjm's suggestion to wait until A-C and D-G patches are committed. Whatever list changes you correct I will simply take out of the follow up list patch whenever this get ins.
Comment #92
xjmExcellent, thanks @Lars Toomre. :)
Comment #93
jhodgdonFrom the interdiff:
I think the @param $key got added to the wrong function here?
The rest looks good...
Comment #94
xjmHum, guess I got confused and thought both those functions took the key. Fixed here.
Comment #95
jhodgdonGreat! I think we're good to go now.
Comment #96
catchThanks! Committed/pushed to 8.x.
Comment #98
xjmResurrecting for as much D7 backport as is appropriate.
Comment #99
xjmHere's a backport with the new formatting for menu callbacks, etc. removed. There were also a few methods in
cache.inc
that don't exist in D7. I diffed the patches manually to show the differences.Comment #100
xjmErr, ignore that last interdiff. That was against the modified patch where I'd already removed the menu callbacks. ;) This is against the original.
Comment #101
aspilicious CreditAttribution: aspilicious commentedOk done with this huge patch...
Comment #102
webchickCommitted and pushed to 7.x. Thanks!
Comment #104
xjmComment #104.0
xjm.