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 consistent quote style #1545

Closed
wants to merge 1 commit into from
Closed

Conversation

benthorner
Copy link
Contributor

@benthorner benthorner commented Sep 12, 2019

Previously we allowed any style of quotation between files and within
the same file, which only serves to increase the mental load when trying
to write consistent code. It would be quite a large change to reenable
the associated RuboCop across the whole of GOV.UK, but this change will
help reduce the quotation choice overhead for at least this repo.

Search page examples to sanity check:

Other finders

@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1545 September 12, 2019 10:15 Inactive
@benthorner benthorner force-pushed the enforce-consistent-quotes branch from 4079521 to dc87f31 Compare September 12, 2019 10:22
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1545 September 12, 2019 10:22 Inactive
@theseanything
Copy link
Contributor

Ah could the commit message / PR description have the reason for the change?

Previously we allowed any style of quotation between files and within
the same file, which only serves to increase the mental load when trying
to write consistent code. It would be quite a large change to reenable
the associated RuboCop across the whole of GOV.UK, but this change will
help reduce the quotation choice overhead for at least this repo.
@benthorner
Copy link
Contributor Author

I guess not everyone sees is as 'anarchy'. Updated.

@koetsier
Copy link
Contributor

koetsier commented Sep 12, 2019

Double quotes are usually used for strings you can interpolate and single quotes for ones you don't - at least that's what I've always used as a guideline as to when to use double/single quotes. So I'd prefer single quotes where possible. (That's also what my RubyMine linter says)

@benthorner
Copy link
Contributor Author

@koetsier I think this may have come up before ;-).

rubocop/rubocop#5306

We also have something of a precedent for this...

alphagov/content-publisher#169

@kevindew
Copy link
Member

While this may risk all hell breaking loose, how about we make this a change in https://github.com/alphagov/govuk-lint ? Though it should have some time for some conversation.

My personal view is anyone who has a preference on either type are wrong, it doesn't matter. But having no consistency, or changing consistency from file-to-file, is annoying, confusing and adds an extra decision you have to make before writing any code in a project.

@koetsier
Copy link
Contributor

@kevindew I was not aware this was such a contentious issue!

I don't feel very strongly about this - single, double, some weird mixture -for me personally I don't think it adds mental load but if it does for others I'm happy either way.

@kevindew
Copy link
Member

@kevindew I was not aware this was such a contentious issue!

I don't feel very strongly about this - single, double, some weird mixture -for me personally I don't think it adds mental load but if it does for others I'm happy either way.

Ha yup. It's just one of those classic long term programmer debates like tabs vs spaces, where the debate rages on eternally, with just enough subtleties for preferences to be based on slightly more than aesthetics.

Sorry if my comment came across as an attack on you personally, it was meant as a general comment on the arguments not your particular point.

@benthorner
Copy link
Contributor Author

Closing in favour of alphagov/govuk-lint#124

@benthorner benthorner closed this Sep 13, 2019
@kr8n3r kr8n3r deleted the enforce-consistent-quotes branch December 9, 2019 07:51
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.

5 participants