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

Allow double quotes and template string sometimes #7555

Merged

Conversation

tofumatt
Copy link
Member

@tofumatt tofumatt commented Jun 26, 2018

Description

Fixes #7366.

This PR is two commits:

  1. A simple eslint change to fix Enable avoidEscape in eslint string config so we can avoid escaping single quotes #7366; allow double-quoted and template-string-quoted strings in JS only when there is a single quote inside the string. As mentioned in Enable avoidEscape in eslint string config so we can avoid escaping single quotes #7366, this leads to much more readable strings and I think is nicer. Also imagine the bytes we'll save omitting all those \ 😉
  2. A change of all existing instances in JS that are covered by this to use double quotes

How has this been tested?

Everything still works; eslint doesn't yell at you.

Types of changes

Code quality.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

Obviously this violates https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/#strings, so I guess we should have a chat about this in #core-js or something first. 🤷‍♂️

@tofumatt tofumatt requested a review from a team June 26, 2018 19:05
@aduth
Copy link
Member

aduth commented Jun 26, 2018

so I guess we should have a chat about this in #core-js or something first. 🤷‍♂️

Added to the agenda.

I may be in the minority, but my preference is toward the strict consistency. There's also another point of: When we're using the single quote in strings, particularly localized strings, we're misusing it as an apostrophe, which has its own character that is not prone to the same issues with escaping. We may indirectly make its correct usage more apparent by virtue of the pain involved in otherwise escaping a single quote.

Related: https://english.stackexchange.com/a/36048

@tofumatt
Copy link
Member Author

Thank you!

Yeah, the usage of single-quotes for apostrophe is another thing I'd happily go through and correct.

That's an interesting idea; re: annoyance of needing to escape it minimising the problem. 😄

@aduth
Copy link
Member

aduth commented Jun 26, 2018

To be my own self-critic, I've found myself in a fascinating rabbit hole of a topic surrounding apostrophes vs. single quotes.

The Wikipedia article claims:

The apostrophe looks the same as a closing single quotation mark in many fonts, although they have different meanings, and Unicode recommends using the quotation mark character to represent most uses of the apostrophe.

https://en.wikipedia.org/wiki/Apostrophe

Yet, as best I can tell, the Unicode specification itself says U+2019 () is still the preference:

2019 ’ RIGHT SINGLE QUOTATION MARK
= single comma quotation mark
• this is the preferred character to use for
apostrophe

https://www.unicode.org/charts/PDF/U2000.pdf

Also: https://www.quora.com/Punctuation-Why-is-the-right-single-quote-U+2019-and-not-the-semantically-distinct-apostrophe-U+0027-the-preferred-apostrophe-character-in-Unicode

Anyways, I did not mean to make this a big point, but I felt compelled to share that the issue is more complicated than I might have implied it was 😄

@aduth
Copy link
Member

aduth commented Jun 26, 2018

Still on my tangent: In the same way we have a rule preventing three dots in favor of the ellipsis proper, we could introduce something similar for apostrophe, though it may be hard to tell where (if any) is valid use for the single quotation. Then again, in its rare usage, an inline /* eslint-disable-next-line */ would be fine.

gutenberg/.eslintrc.js

Lines 117 to 120 in d316177

{
selector: 'CallExpression[callee.name=/^(__|_x|_n|_nx)$/] Literal[value=/\\.{3}/]',
message: 'Use ellipsis character (…) in place of three dots',
},

@tofumatt
Copy link
Member Author

I am so into this tangent, fixing those kind of nitpicks gives me much happiness. 😄

@aduth
Copy link
Member

aduth commented Jul 3, 2018

If considered for adoption, this will need to include relevant guideline documentation in the coding-guidelines.md file.

aduth
aduth previously requested changes Jul 3, 2018
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's document this in coding guidelines, including mention of appropriate use of apostrophe.

@ntwb
Copy link
Member

ntwb commented Jul 3, 2018

I think we can probably also adopt this for core coding standards

Once this is in here I'll look at proposing that for core and updating eslint-plugin/config-wordpress and https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/#strings

@tofumatt tofumatt self-assigned this Jul 3, 2018
@tofumatt tofumatt force-pushed the chore/7366-allow-double-quotes-and-template-string-sometimes branch from 87b1672 to 2d10143 Compare July 24, 2018 17:36
@tofumatt tofumatt requested a review from aduth July 24, 2018 23:17
@tofumatt
Copy link
Member Author

Sorry I neglected this; updated the coding standards and tried to go through as much of the code as possible and replace single-quotes that should have been apostrophes with the correct character. Should be set for another review, then we can look at changing the core standards as well... though we should probably chat about whether Gutenberg is the canonical source for core JS standards anyhow, as it's the biggest piece of JS in WordPress 😄

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the fact we don't need to escape those strings anymore, it also makes a lot of sense to use for translations 👍

For tests we probably should pick one approach and be consistent, at the moment it's all random :)

@@ -137,7 +137,7 @@ describe( 'block parser', () => {
} );

describe( 'parseWithAttributeSchema', () => {
it( 'should return the matcher\'s attribute value', () => {
it( "should return the matcher's attribute value", () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would put here, too. To match with other test names :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I didn't want to go too wild with tests but I suppose just being consistent everywhere is cool. Will do!

@@ -571,7 +571,7 @@ describe( 'block parser', () => {
} );

const parsed = parse(
'<!-- wp:core/test-block {"smoked":"yes","url":"http://google.com","chicken":"ribs & \'wings\'"} -->' +
`<!-- wp:core/test-block {"smoked":"yes","url":"http://google.com","chicken":"ribs & 'wings'"} -->` +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an epic string which tricks all standards :trollface:

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

gziolo commented Jul 25, 2018

068fce3 - like it 👍

@tofumatt tofumatt dismissed aduth’s stale review July 25, 2018 11:29

Mentioned in the coding guidelines, so I think we're good 🙂

@tofumatt tofumatt removed the request for review from aduth July 25, 2018 11:30
@tofumatt tofumatt merged commit 804abec into master Jul 25, 2018
@tofumatt tofumatt deleted the chore/7366-allow-double-quotes-and-template-string-sometimes branch July 25, 2018 11:56
@aduth
Copy link
Member

aduth commented Sep 18, 2018

Counter-point in the wild: In copying and modifying existing code which is allowed to be exempted, it's not clear to a developer that they'd need to change between single and double quotes: 27b4423 #5476 (comment) (assumed to be copied by recommendation in #5476 (comment) )

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

Successfully merging this pull request may close these issues.

Enable avoidEscape in eslint string config so we can avoid escaping single quotes
4 participants