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

Enforce single quotes and disallow var in unit tests #4154

Merged
merged 2 commits into from
Mar 28, 2022
Merged

Conversation

willeastcott
Copy link
Contributor

@willeastcott willeastcott commented Mar 27, 2022

  • Enforce single quotes for all node-based unit tests (note that single quotes are used over double quotes in our codebase by over 2 to 1). I would like to see this standardized across the codebase.
  • Migrate all occurrences of var to let and const.

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

@willeastcott willeastcott requested a review from a team March 27, 2022 01:34
@willeastcott willeastcott self-assigned this Mar 27, 2022
@willeastcott willeastcott changed the title Enforce single quotes in unit tests Enforce single quotes and disallow var in unit tests Mar 27, 2022
@slimbuck
Copy link
Member

Perhaps we should standardise on ` instead?

@willeastcott
Copy link
Contributor Author

willeastcott commented Mar 28, 2022

@slimbuck That's a reasonable suggestion. However, I kind of like reserving ` for template string literals. For example, say there was a long string and I saw it started with a `, I would then have to scan the whole string to find the embedded variables which may or may not be there.

@slimbuck
Copy link
Member

Yeah makes sense. ' is really similar to ` - perhaps " would be a better standard?

@willeastcott
Copy link
Contributor Author

I mainly went for single because:

  • It appears twice as many times in our source as double (so we'll generate fewer changes when we standardize, plus people obviously prefer single over double if that's the statistic)
  • single is one keypress, double is two. 😉

Yeah, double is visually differentiated a bit more. But does that outweigh the above? I'm not totally sold.

@slimbuck
Copy link
Member

I would say readability of the codebase is more important than either the one-off change or double keystroke.

@slimbuck
Copy link
Member

(and tbh I think there's a big difference in readability between " and ' vs `).

@willeastcott
Copy link
Contributor Author

Actually, just remembered that we unofficially stick to Google's JS coding standards for the most part:

https://google.github.io/styleguide/jsguide.html#features-strings-use-single-quotes

So that's another key reason why you'll find way more singles than doubles right now. We just never enforced it.

@willeastcott
Copy link
Contributor Author

Anybody else feel strongly either way?

@mvaligursky
Copy link
Contributor

Personally I tend to use " for these main reasons, but no super strong preference:

  • I find it easier to read, as ' vs ` is getting less obvious
  • I'm used to it from c++ and c# and it feels right. My mind is wired to use ' for a single char type only
  • the plugin (or built in functionality?) that VSCode has for managing imports uses " in import statements: import { Vec3 } from "../../math/vec3.js";

@willeastcott
Copy link
Contributor Author

the plugin (or built in functionality?) that VSCode has for managing imports uses " in import statements: import { Vec3 } from "../../math/vec3.js";

If I'm following what you're talking about here, I believe VS Code is smart enough to check the quotes of other imports in the file and try to match them.

quotes

@mvaligursky
Copy link
Contributor

Great! Sounds like that one is a non issue then.

@Maksims
Copy link
Contributor

Maksims commented Mar 28, 2022

A single quote is easier to type, as no extra button press is required.
Template literals should be used only where it is required, linters in IDE can even highlight strings that use ` but do not utilize it with actual templating.
Another tiny point is single quote is the same key as US and UK keyboard layout, while the double quote is not.

@willeastcott
Copy link
Contributor Author

Haha, yeah, I can relate to that. I have a US keyboard at home and a UK keyboard in the office!

Copy link
Member

@slimbuck slimbuck left a comment

Choose a reason for hiding this comment

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

We chatted internally and decided single quote is the way to go.

@willeastcott willeastcott merged commit 53fd940 into main Mar 28, 2022
@willeastcott willeastcott deleted the test-quotes branch March 29, 2022 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants