Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various variables do not use proper camelCase for instances of the words “ID” and “UID” #6741

Closed
ZebulanStanphill opened this issue May 14, 2018 · 12 comments
Labels
[Type] Code Quality Issues or PRs that relate to code quality

Comments

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented May 14, 2018

The issue

In some of the files in the code, I noticed that firstUid, lastUid, noticeId and updatedId

See:
https://github.com/WordPress/gutenberg/search?utf8=%E2%9C%93&q=firstUid
https://github.com/WordPress/gutenberg/search?utf8=%E2%9C%93&q=lastUid
https://github.com/WordPress/gutenberg/search?utf8=%E2%9C%93&q=updatedId

https://github.com/WordPress/gutenberg/blob/63abd32d9b5cb809ad71167f209dd7a6d0a79a3f/editor/components/block-mover/index.js
https://github.com/WordPress/gutenberg/blob/63abd32d9b5cb809ad71167f209dd7a6d0a79a3f/editor/store/test/actions.js
https://github.com/WordPress/gutenberg/blob/63abd32d9b5cb809ad71167f209dd7a6d0a79a3f/editor/store/actions.js
https://github.com/WordPress/gutenberg/blob/c45d6bd5380f2e7a9a5d9ce82b3fcb845fb715b4/editor/store/reducer.js
https://github.com/WordPress/gutenberg/blob/c45d6bd5380f2e7a9a5d9ce82b3fcb845fb715b4/editor/store/test/reducer.js
https://github.com/WordPress/gutenberg/blob/fb804e28585be0d7346953707c03080bfa5d604d/editor/store/effects.js
https://github.com/WordPress/gutenberg/blob/63abd32d9b5cb809ad71167f209dd7a6d0a79a3f/editor/store/test/effects.js

From #6742:

withInstanceId uses incorrect capitalization of “ID”. It should be “withInstanceID”. A variable used by the component, instanceId also uses incorrect capitalization.
https://github.com/WordPress/gutenberg/tree/master/components/higher-order/with-instance-id

Assuming I understand camelCase properly, “ID” and “UID” should always be entirely capitalized unless it is the first word in a name, in which case it should be entirely lowercase.

Expected behavior

“ID” and "UID" should be correctly capitalized in all functions, variables, etc. according to camelCase standards.

Related Issues and/or PRs

#6685
#6742

@ZebulanStanphill ZebulanStanphill changed the title Various variables do not use proper camelCase for instances of the words "ID" and "UID" Various variables do not use proper camelCase for instances of the words “ID” and “UID” May 14, 2018
@chrisvanpatten
Copy link
Member

chrisvanpatten commented May 14, 2018

eslint's camelcase rule prefers Id over ID and while UID vs Uid (or UId) is a bit more ambiguous, Uid seems more consistent.

EDIT: That is to say, eslint should probably be trusted as an authoritative source on this, and this should not be changed.

@danielbachhuber
Copy link
Member

eslint's camelcase rule prefers Id over ID and while UID vs Uid (or UId) is a bit more ambiguous, Uid seems more consistent.

@aduth @youknowriad Do either of you have an opinion on this?

@aduth
Copy link
Member

aduth commented May 14, 2018

Previously: #2511

@aduth
Copy link
Member

aduth commented May 14, 2018

My latest personal leanings based on consistency with the broader ecosystem at large is to capitalize acronyms, and camelcase everything else.

Examples:

  • userId (Id is an abbreviation for "Identifier")
  • getDOMNode (DOM is an acronym for "Document Object Model")

UID is a tricky one, since it's partly both acronym and abbreviation ("Unique Identifier"). My recommendation would be... maybe just call it an identifier 😄

@gziolo gziolo added the [Type] Code Quality Issues or PRs that relate to code quality label May 15, 2018
@danielbachhuber
Copy link
Member

@aduth To avoid bikeshedding again, I suggest we just go for it. Can we enforce these in linting rules?

@robertdevore
Copy link
Contributor

@danielbachhuber this will revert this merge then, correct???

@danielbachhuber
Copy link
Member

@robertdevore Yep. But we need a final say on the direction we want to head.

@aduth
Copy link
Member

aduth commented May 24, 2018

Can we enforce these in linting rules?

Not as proposed in #6741 (comment), since it would rely on an understanding of what is and isn't an acronym or abbreviation. If we subscribed to the idea that all things be camel-cased, we could set a rule that a variable must begin with a lower-cased letter and, if a capital letter is ever encountered, it can't be followed by anything other than a lowercase letter.

We'd also need to consider things like classes, where PascalCase is the norm (though not documented as such in coding standards, perhaps because class, albeit sugar on the prototype pattern, was not standard/common until ES2015). Also things like constants, where PHP naming conventions allow for SCREAMING_SNAKE_CASE and there is some precedent of this in Gutenberg, though again not explicitly documented in coding standards.

If we want to avoid bikeshed, here's my (opinionated) suggestions:

  • camelCase for variables
    • Exception / Clarification: Acronyms must be capitalized, reflecting the fact that in its expanded form each is a separate word (e.g. getDOMNode = "get document object model node")
    • Cannot be enforced by ESLint
  • PascalCase for class
    • Exception / Clarification: Any WordPress (React) component must be assigned with PascalCase, including stateless function components, for consistency's sake and in being accommodating to refactoring to/from class extends Component
    • Can be enforced by ESLint for class, not for stateless function components
  • SCREAMING_SNAKE_CASE to indicate a true constant: an unchanging and immutable (by convention, not technically enforced) value defined in the top-most scope of a file
    • Note contrast with const statement which does not imply immutability.
      • The const declaration creates a read-only reference to a value. It does not mean the value it holds is immutable, just that the variable identifier cannot be reassigned.

    • Prohibitively difficult though perhaps not impossible to enforce via ESLint by detecting assignment into property (const PI = 3.14159; PI.foo = 10 /* error */;)

@tofumatt
Copy link
Member

tofumatt commented Jul 2, 2018

I find @aduth's suggestion fine; it's what I'm used to in other JS projects.

For UID vs UId I think the argument that the latter is more technically correct is outweighed by the fact that it's weird and kinda an acronym itself. Languages are a system of rules/conventions with the odd exception: I say let UID be our exception to the rule 😉

@aduth
Copy link
Member

aduth commented Jul 2, 2018

Raised again in Slack to be addressed:

https://wordpress.slack.com/archives/C02QB2JS7/p1530544700000001

I don't love the idea of an exception, as it seems harder to enforce consistently. There's been some struggle on the naming of a block's uid vs. id (the latter is used as the prop name to a block's edit, the former in all other cases). In the previous conversation, @mtias had mentioned that one of the goals with having a distinct name was making it clear that it is not persisted, and is unique from an "ID" as referenced elsewhere in WordPress codebase / database ([1], [2]).

It's for this reason I'm wondering if we could be more explicit here while eliminating the chance for ambiguity on capitalization, calling it instead a clientId.

@tofumatt
Copy link
Member

tofumatt commented Jul 2, 2018

UID is a weird convention in software anyway, so I like your solution 👍

@noisysocks
Copy link
Member

#7669 addressed this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

No branches or pull requests

8 participants