Fixing newlines

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

These items are missing the second line of the @return (the description):

  /**
    * Checks if the project is installed.
+   *
    * @return bool
    */
   public function isInstalled();

    *        provide their canonical name.
    *
    * @param string $directory
+   *
    * @return string
    */
   public static function getProjectName($directory) {


    *
    * @param string $directory
    *   Directory to search for the info file.
+   *
    * @return string
    */
   public static function getProjectTitle($directory) {

Is the indentation also off on the bit above that second one?

aspilicious’s picture

First 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 ;)

jhodgdon’s picture

@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...
:)

aspilicious’s picture

New try

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

looks good!

Dries’s picture

Can we combine all newline patches into one patch, please?

webchick’s picture

While 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.

aspilicious’s picture

I 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 ;).

webchick’s picture

We 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).

jhodgdon’s picture

The 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".

aspilicious’s picture

I agree, next time I'll push it to D8 if I feel like fixing docs...

jhodgdon’s picture

aspilicious: 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...

jhodgdon’s picture

Status: Reviewed & tested by the community » Postponed
jhodgdon’s picture

jhodgdon’s picture

Title: Fix newlines in updater.inc » Add missing doc and newlines in updater.inc
Status: Postponed » Reviewed & tested by the community

I'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...

Dries’s picture

Oddly, 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. ;-)

jhodgdon’s picture

The patch in #4 applies fine for me. Maybe you can try again when you get to a better internet connection? :)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.