Make WordPress Core

Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#43041 closed enhancement (fixed)

Rename the capital_P_dangit function

Reported by: danieltj's profile danieltj Owned by: jrf's profile jrf
Milestone: 5.1 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch
Focuses: coding-standards Cc:

Description (last modified by johnbillion)

I'm not too bothered about the capital_P_dangit function in terms of what it does. Correcting WordPress to WordPress is fine by me. Capitalise the P and it never worries you.

My problem with this function is the name and documentation of it. Taken from a 4.9.1 install:

wp-includes/formatting.php, line 4805:

Forever eliminate "Wordpress" from the planet (or at least the little bit we can influence).

Violating our coding standards for a good function name.

As funny as the name might seem the first time around, violating the coding standards doesn't seem like something that should live on in core for the sake of a gag. I'd like to propose that capital_P_dangit is deprecated and used as a wrapper function for a newly named wp_brand_awareness.

I personally think wp_brand_awareness is a better name, fits in with coding standards and still does what it's set out to do, but in a more 'WordPress-esque' way. I'd also suggest updating the documentation to remove the comment about from our planet and for a gag lines too.

Attachments (1)

42429-re-able-sniff-after-disabling.patch (770 bytes) - added by jrf 7 years ago.
Add-on to committed patch

Download all attachments as: .zip

Change History (12)

#1 follow-up: @pento
7 years ago

  • Milestone changed from Awaiting Review to 5.0
  • Owner set to pento
  • Status changed from new to assigned
  • Version 4.9.1 deleted

Renaming it is definitely out, but I agree that the coding standards warnings are a problem. Let's see what I can do... 😉

#2 @pento
7 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 42429:

Formatting: Fix capital_P_dangit() not capital P-ing, dangit.

Unfortunately, in my overzealous desire to fix all the un-capitalised Ps, [42343] capitalled some Ps that intentionally should not be capitals.

This commit fixes the incorrectly capitalling of Ps, as well as including unit tests and PHPCS instructions to ensure we never capitulate to capitalisationing them again.

Props superdav42.
Fixes #43040.
Fixes #43041.

#3 @pento
7 years ago

😁

@jrf
7 years ago

Add-on to committed patch

#4 @jrf
7 years ago

@pento phpcs:disable/set annotations are not file-specific, so always need to be followed by a phpcs:enable/set command to reset to the original ruleset.

With phpcs:set, this would be a set annotation resetting the value to the default or the value as set by the WPCS ruleset.

I've attached a patch to fix this for the new unit test file.

#5 follow-up: @pento
7 years ago

Huh, interesting. I tested for that, and it seemed to automatically be re-enabled in a subsequent file.

#6 @pento
7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#7 @pento
7 years ago

  • Keywords has-patch commit added
  • Owner changed from pento to jrf
  • Status changed from reopened to assigned

#8 @jrf
7 years ago

Huh, interesting. I tested for that, and it seemed to automatically be re-enabled in a subsequent file.

Hmm.. something might have changed with the new annotations in that case. I'll have a further look.
I know for sure that the old style annotations are not file-specific as we've run into this exact issue when unit testing sniffs.

Also - the file order is not predetermined and can be different across systems, quite apart from caching and parallel processing influencing the results, so the end-results may be misleading.

All the same, resetting/re-enabling whenever these annotations are used is never a bad habit to get used to.

#9 in reply to: ↑ 1 @danieltj
7 years ago

Replying to pento:

Renaming it is definitely out, but I agree that the coding standards warnings are a problem. Let's see what I can do... 😉

How come?

The description even says:

Violating our coding standards for a good function name.

Surely the idea is to ensure everything is kept consistent across the code base? I'd insist that it's changed to be more appropriate as whilst it's amusing to begin with, violating code standards for a joke wouldn't be tolerated elsewhere.

#10 @johnbillion
6 years ago

  • Description modified (diff)
  • Keywords commit removed
  • Milestone changed from 5.0 to 5.1
  • Resolution set to fixed
  • Status changed from assigned to closed

I agree with Pento here. Renaming this function introduces backwards compatibility issues and is a breaking change. Deprecating it in favour of a better named function is more effort than it's worth.

Closing as fixed for 5.1 due to the sniffer comments introduced in [42429].

#11 in reply to: ↑ 5 @SergeyBiryukov
6 years ago

Huh, interesting. I tested for that, and it seemed to automatically be re-enabled in a subsequent file.

Just wanted to confirm that. Per https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage:

Note: All phpcs:disable and phpcs:enable comments only apply to the file they are contained within. After the file has finished processing all sniffs are re-enabled for future files.

Note: See TracTickets for help on using tickets.