Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Fixing newlines
Comment | File | Size | Author |
---|---|---|---|
#4 | newline_updater_inc_v2.patch | 1.7 KB | aspilicious |
newline_updater_inc.patch | 1.36 KB | aspilicious | |
Comments
Comment #1
jhodgdonThese items are missing the second line of the @return (the description):
Is the indentation also off on the bit above that second one?
Comment #2
aspilicious CreditAttribution: aspilicious commentedFirst one is returning a bool, true is the project is installed, false if not
Second one is returning the project name
Third is the project title
If someone would be nice to make nice sentences of these then I'll roll another patch ;)
Comment #3
jhodgdon@return bool TRUE if the project is installed, FALSE if not.
@return string The name of the project.
@return string The title of the project.
They don't actually have to be complete sentences...
:)
Comment #4
aspilicious CreditAttribution: aspilicious commentedNew try
Comment #5
jhodgdonlooks good!
Comment #6
Dries CreditAttribution: Dries commentedCan we combine all newline patches into one patch, please?
Comment #7
webchickWhile I really do appreciate the sentiment going into this sort of garden tending, realize that this work is currently totally clogging up the RTBC queue when there are numerous, beta-blocking patches sitting there needing attention, and our focus needs to be on fixing those. Furthermore, applying such patches unless they're rolled approximately 1 hour before a 7.0 release ends up being largely ineffectual, as they break other patches waiting for review/commit, which, once re-rolled and finally committed, will inevitably end up introducing additional newline problems, which will in turn require more of these kinds of clean up patches, ad infinitum...
If you're passionate about fixing this, I think a better approach would be to come up with a shell script that core maintainers can run just before a release as a general clean-up.
Comment #8
aspilicious CreditAttribution: aspilicious commentedI didn't ask for these minor patches to be commited so fast...
I agree with some kind of script. But I'm afraid some coders will ignore standards cause the script will correct it.
These cleanups are needed cause currently coders are copying doxygen code with errors and use that style to write new one.
I'm ok with not committing these patches right now.
If these patches broke some other patches, I'm willing to reroll them ;).
Comment #9
webchickWe are definitely not going to apply and commit 300 separate patches, and mark 300 separate issues as fixed, as this would add approximately 600 minutes in overhead, so what Dries said in #7 stands. Make one issue to track the whitespace problems with @param and @return, mark the rest of them as duplicates.
I'm a little dubious of the timing of this entire initiative, though. Were these new whitespace rules agreed upon before code freeze (and if so, where's the issue where this was done)? If not, this qualifies as an "API change" to me, since module developers would need to make similar 500K patches to their own modules to conform. Smells very much like D8, given that we already had a 3-month "polish" phase for this kind of gardening (once again, we're trying to get D7 out the door, thus focusing on beta-blocking patches).
Comment #10
jhodgdonThe standard has been around for quite a while. Here's the issue where it was discussed:
#550942: Blank line between @param and @return
It was agreed upon by webchick, sun, and merlinofchaos in August of 2009, and added to the standards page by effulgentia in October of 2009. At which point the issue was moved to the coder module so that they could flag the error. Core was not patched.
But I agree, this is far from being the most important thing to fix in core right now, or even to fix in core documentation. And there is no official "initiative".
Comment #11
aspilicious CreditAttribution: aspilicious commentedI agree, next time I'll push it to D8 if I feel like fixing docs...
Comment #12
jhodgdonaspilicious: I don't think you need to push all ideas of doc fixing off to D8. There are plenty of documentation issues that are active and need patching...
Comment #13
jhodgdonComment #14
jhodgdonpostponed as per http://drupal.org/node/855410#comment-3208874 and http://drupal.org/node/855324#comment-3208840
Comment #15
jhodgdonI've postponed the other only-newlines issues...
However, this one adds a bunch of missing doc lines, so can we make an exception and commit this one? Thanks...
Comment #16
Dries CreditAttribution: Dries commentedOddly, I can't seem to apply the patch.
deimos:drupal-cvs dries$ patch -p0 < f.p
patch unexpectedly ends in middle of line
patch: **** Only garbage was found in the patch input.
I'm reviewing patches from an airplane over a sketchy internet connection -- maybe that has something to do with it. ;-)
Comment #17
jhodgdonThe patch in #4 applies fine for me. Maybe you can try again when you get to a better internet connection? :)
Comment #18
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.