Skip to content
This repository has been archived by the owner on Jan 6, 2020. It is now read-only.

Revert "Don't enforce double-quotes for strings." #124

Merged
merged 1 commit into from
Sep 16, 2019

Conversation

benthorner
Copy link
Contributor

@benthorner benthorner commented Sep 13, 2019

This reverts commit 67e3ea7. Based on discussions in other repos [1][2] and what seems to be general agreement in the community, I think we should reenable this and eliminate the mass inconsistency that's crept into a lot of repos since 2015. It's one of those little pain points that keeps coming up. In contrast to the original PR, I think disabling the cop has actually made it harder to be consistent.

To quote @kevindew:

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.

The good news is that it's very easy to automatically fix this across an entire repo, which I'm aware is one of the consequences of merging this PR, and which I'm happy to help with.

Copy link
Contributor

@thomasleese thomasleese left a comment

Choose a reason for hiding this comment

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

I'm happy with this change.

@kevindew
Copy link
Member

I'm all for this 👍

This PR should update the changelog too - likely with instruction of how to apply the fix globally and programatically.

If we don't hit any major points of contention getting this PR in I wonder if we should also include the re-instating of the trailing comma change in the next major release - this was the other old rule that we trialled in Content Publisher and didn't seem to upset anyone.

@benthorner benthorner force-pushed the enforce-consistent-quotes branch from a966c38 to 8c38466 Compare September 16, 2019 09:05
@benthorner benthorner force-pushed the enforce-consistent-quotes branch from 8c38466 to cc3eeeb Compare September 16, 2019 09:12
@benthorner
Copy link
Contributor Author

@kevindew I noticed that when I was looking for which commit to revert. It seems less clear-cut, historically, than this change, partly due to the cops being renamed and split up. The closest thing I can find is 5e42ec5 - do you know if there's a more definite commit we could refer to?

@kevindew
Copy link
Member

@kevindew I noticed that when I was looking for which commit to revert. It seems less clear-cut, historically, than this change, partly due to the cops being renamed and split up. The closest thing I can find is 5e42ec5 - do you know if there's a more definite commit we could refer to?

Ah I had thought it was set to true and the false given the extra options that cop had. However there is this: alphagov/styleguides@e197dce although since we apply the symmetrical style to MultiLineMethodLayout the bad example now feels out of date compared to what we regularly use.

@benthorner benthorner merged commit 0cbf19c into master Sep 16, 2019
@benthorner benthorner deleted the enforce-consistent-quotes branch September 16, 2019 14:40
@benthorner
Copy link
Contributor Author

@kevindew thanks for the references. I there are two separable parts in alphagov/styleguides@e197dce: the parenthesis layout; and the trailing commas. On that basis, it seems like there's a fair precedent to (re?)introduce the comma aspect. Again, there's the same argument around needless flexibility. I'll do a PR in a bit...

@benthorner
Copy link
Contributor Author

Aha! I found it: #125

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants