WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.36k stars 4.14k forks source link

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

Closed ZebulanStanphill closed 6 years ago

ZebulanStanphill commented 6 years ago

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

chrisvanpatten commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Previously: #2511

aduth commented 6 years ago

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

Examples:

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 😄

danielbachhuber commented 6 years ago

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

robertdevore commented 6 years ago

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

danielbachhuber commented 6 years ago

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

aduth commented 6 years ago

Can we enforce these in linting rules?

Not as proposed in https://github.com/WordPress/gutenberg/issues/6741#issuecomment-388873527, 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:

tofumatt commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

noisysocks commented 6 years ago

7669 addressed this.