Site-kit-wp: Consistently name acronyms inside variables

Created on 17 Oct 2019  ·  15Comments  ·  Source: google/site-kit-wp

Feature Description

In PHP code, we typically use snake-case which has abbreviations typically all-lowercase (e.g. $site_url. In PHP classes, we typically capitalize them (e.g. class Site_URL).

In JS however, and in PHP API responses (to be consumed by JS), we're inconsistent. We _consistently_ use camel-case and should continue to do so, but inside those camel-case variables we sometimes use siteUrl, other times siteURL.

Let's decide on and apply a common standard for how acronyms should be treated inside camel-case variable names.

_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance criteria

  • The following abbreviations should be consistently used throughout all camel-case variables and JS class names in the plugin (except cases where another format is required due to third-party code/APIs):

    • URL

    • URI

    • ID

Implementation Brief

  • Search for compound forms of URL, URI, and ID (excluding vendor directories)
    Regex: \w(Url|Uri|Id) (make sure to enable case-sensitivity)

    • Should be all caps when part of a variable or method name

  • Search for single word forms of URL, URI, and ID (excluding vendor directories)
    Regex: \b(URL|URI|ID)\b (make sure to enable case-sensitivity)

    • Should be all lower case when used as a word by itself

  • Update matches in JS and JS-related PHP (such as API responses and localized data that are consumed by JS)
  • Only update Site Kit owned code - care must be taken to not apply these changes to 3rd party code which requires a different casing or to code which makes 3rd party API requests

Changelog entry

  • Consistently capitalize acronyms in camel-case API parameters, variables, etc.
P1 Bug
Source
picture felixarntz

All 15 comments

@aaemnnosttv @adamsilverstein @tofumatt Even Gutenberg appears to be inconsistent here, I can't really tell a preference. Let's discuss on this issue and make a decision. I'll then update the ACs to the respective variant we decide on.

felixarntz picture felixarntz on 17 Oct 2019
👍1

My usual preference is to capitalise the first letter in both words and acronyms (like URL and URI), because it makes for variables like siteUrlForActivation instead of `siteURLForActivation—the former is easier to pick out individual words/sections in than the one with many subsequent capitals.

So I think the Url style beats out URL, even though I personally actually think URL looks prettier (and is pedantically more _correct_ as URL is an acronym)… it's less easy to _read_. Since I'm a fan of readability and consistent rules more than anything else, I think removing ambiguity and going with "first letter of abbreviations and words" as a rule makes things simple, consistent, and easy.

tofumatt picture tofumatt on 17 Oct 2019

I tend to agree with @tofumatt but luckily there are existing style guides to help inform this decision.

The WordPress Javascript coding standards are fairly clear about this.

Acronyms must be written with each of its composing letters capitalized. This is intended to reflect that each letter of the acronym is a proper word in its expanded form.

All other abbreviations must be written as camel case, with an initial capitalized letter followed by lowercase letters.

I think the WordPress standards are likely sufficient here but should there be any ambiguity left, we can fallback to Google's Javascript style guide which also specifies how to handle acronyms in it's definition of camel case:
https://google.github.io/styleguide/jsguide.html#naming-camel-case-defined

aaemnnosttv picture aaemnnosttv on 18 Oct 2019

Those sorts of rules always get confusing for ESL speakers or folks unfamiliar with certain acronyms. Again: I'm a native English speaker and a closet pedant so I understand the spirit of that rule, but my experience is that it's incredibly inconsistently-applied in practice and confusing for people whose second or third language is English. I think that's an important open-source consideration, and an instance where the WP coding standards gets it wrong :-)

The WP Standards require a lot more internalised knowledge of both technical and language nuances… and also require more human consideration/review. Applying a simpler rule (that you also agree with) means less developer time wasted on thinking whether ID stands for something or doesn't. :D

tofumatt picture tofumatt on 18 Oct 2019

I generally favor the "all caps" acronym approach outlined in the WordPress JavaScript Coding standards, although I am swayed by the argument that this makes deciding what to capitalize harder especially for non english native speakers. I'm not sure I find one or the other more readable.

Overall, I feel we should be guided by the WordPress coding standards (capitalizing acronyms) since that is what the project as a whole follows.

@aaemnnosttv Thanks for the link to the Google JS style guide, I hadn't seen that before. I do appreciate the more deterministic approach taken there to camel casing...

adamsilverstein picture adamsilverstein on 18 Oct 2019

Regarding the Google style guide, interestingly enough in Go, which is heavily used by Google, it is preferred to use all-caps acronyms even in camel-case names.

I think the WordPress coding standards argument pretty much settles it for now, since that should our guiding point on code style. Let's go with the all-caps variant (of course except when it's the only word (as in a variable should be called url, not URL).

felixarntz picture felixarntz on 20 Oct 2019
👍1

May likely end up as a XXS but allocating a bit more time due to the amount of code/files that will likely change.

aaemnnosttv picture aaemnnosttv on 21 Oct 2019

IB ✅

felixarntz picture felixarntz on 21 Oct 2019

QA ⚠️

Missed changes

Any exceptions to the casing rules should be documented inline as to why to save time going back to see why it is the way it is. This was not part of the AC but I think this should be done to support future review and.

aaemnnosttv picture aaemnnosttv on 25 Oct 2019
👍1

@felixarntz can you advise how we should handle the more ambiguous situations mentioned in my comment above? Should we update responses that return Google model instances that use casing that conflict with our rules? See lines about matchedProperty and Tag manager's GET accounts-containers.

aaemnnosttv picture aaemnnosttv on 25 Oct 2019

@aaemnnosttv @adamsilverstein I think, as unfortunate that is, let's stay with the inconsistencies that result from third-party code and APIs. Let's follow the conventions where we can, but third-party code should be left untouched, and renaming API response parameters leads us down a rabbit hole.

felixarntz picture felixarntz on 25 Oct 2019
👍1

Thanks for the review @aaemnnosttv. I'll review the missing changes, fix any that can be updated, and add inline comments to instances that are dealing with API returned data (and thus remain unchanged).

adamsilverstein picture adamsilverstein on 25 Oct 2019

@felixarntz I'm not sure about changes like this one ampExperimentJson -> ampExperimentJSON. This is not part of the AC but of course the idea is the same.

It raises another question of casing though: should we be capitalizing other acronyms: JSON and AMP?

Also, should we be using JS casing for our option keys? This has resulted in a lot of migration handling, but I wonder if it's better to just force snake case for all option keys and only use JS casing between front and back end (localization and datapoint handlers only).

e.g. https://github.com/google/site-kit-wp/blob/99f4ee2dbd61cf86c3ee71663fd981129378c3a8/includes/Modules/Optimize.php#L263-L282

aaemnnosttv picture aaemnnosttv on 28 Oct 2019

@aaemnnosttv While not in the ACs, capitalizing JSON makes sense for the same reasons, as you say. Regarding AMP, we already capitalize that accordingly, there's only one occurrence of Amp, but that is due to third-party code.

Let's not change the option names at this point. Potentially it would have been better to use snake case for them (also since that's the way they're typically named), however if we do this at some point, it would need a severe upgrade routine (in addition to all the stuff we already have now). We can keep this in the back of our heads for a future discussion, but I think the trade-offs outweigh the benefits here.

felixarntz picture felixarntz on 28 Oct 2019
👍1

QA ✅

felixarntz picture felixarntz on 28 Oct 2019
Was this page helpful?
0 / 5 - 0 ratings

Related issues

aaemnnosttv picture aaemnnosttv  ·  3Comments
tofumatt picture tofumatt  ·  3Comments
aaemnnosttv picture aaemnnosttv  ·  5Comments
felixarntz picture felixarntz  ·  4Comments
theeducatedbarfly picture theeducatedbarfly  ·  4Comments