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

fixed hash syntax to Ruby 1.9 #1867

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@askl56

askl56 commented Jul 30, 2015

No description provided.

@floehopper

This comment has been minimized.

Show comment
Hide comment
@floehopper

floehopper Aug 5, 2015

Contributor

@askl56: Thanks for the PR. Unless I'm missing something, it looks as if your changes only address some of the old-style hash syntax. If we're going to make a change like this, I'd prefer to change all of them. Also I'd prefer to introduce Rubocop (or similar) to enforce the use of the new syntax as part of the same set of changes.

Out of interest, do you have a specific motivation for making these changes? Are you using this code yourself?

Contributor

floehopper commented Aug 5, 2015

@askl56: Thanks for the PR. Unless I'm missing something, it looks as if your changes only address some of the old-style hash syntax. If we're going to make a change like this, I'd prefer to change all of them. Also I'd prefer to introduce Rubocop (or similar) to enforce the use of the new syntax as part of the same set of changes.

Out of interest, do you have a specific motivation for making these changes? Are you using this code yourself?

@askl56

This comment has been minimized.

Show comment
Hide comment
@askl56

askl56 Aug 5, 2015

I've added a rudimentary Rubocop sweep which should have caught any outliers. No motivation, beyond trying to help provide consistency and help where I can.

askl56 commented Aug 5, 2015

I've added a rudimentary Rubocop sweep which should have caught any outliers. No motivation, beyond trying to help provide consistency and help where I can.

@chrisroos

This comment has been minimized.

Show comment
Hide comment
@chrisroos

chrisroos Aug 11, 2015

Contributor

Hey @askl56. Thanks for trying to help out.

I can see that you've added a separate commit containing rubocop fixes. Unfortunately, this also contains fixes for a number of other Rubocop violations (e.g. it switches us to use do ... end instead of { ... } for multiline blocks) instead of just changing the hash syntax. If we're to consider merging this PR then it'll need to be focussed on just changing the hash syntax.

In addition: The commit message should be written in the present tense as per our Git Styleguide (i.e. "Fix rather than Fixed"), and I think the commit message should mention our Ruby Styleguide as that's what tells us to "Use Ruby 1.9 syntax for symbolic hash keys".

Contributor

chrisroos commented Aug 11, 2015

Hey @askl56. Thanks for trying to help out.

I can see that you've added a separate commit containing rubocop fixes. Unfortunately, this also contains fixes for a number of other Rubocop violations (e.g. it switches us to use do ... end instead of { ... } for multiline blocks) instead of just changing the hash syntax. If we're to consider merging this PR then it'll need to be focussed on just changing the hash syntax.

In addition: The commit message should be written in the present tense as per our Git Styleguide (i.e. "Fix rather than Fixed"), and I think the commit message should mention our Ruby Styleguide as that's what tells us to "Use Ruby 1.9 syntax for symbolic hash keys".

@floehopper

This comment has been minimized.

Show comment
Hide comment
@floehopper

floehopper Sep 8, 2015

Contributor

@askl56: Hi Andrew. Thanks again for the PR. @chrisroos recently setup the project to use govuk-lint in #1901 & made it run on our CI server in #1911. I've just used Rubocop's auto-correct functionality to change the style across the project - see 4cb96a4. I think that makes this PR redundant and I'm going to close it. Thanks again for your time and I hope it's some consolation that you've encouraged us to fix this!

Contributor

floehopper commented Sep 8, 2015

@askl56: Hi Andrew. Thanks again for the PR. @chrisroos recently setup the project to use govuk-lint in #1901 & made it run on our CI server in #1911. I've just used Rubocop's auto-correct functionality to change the style across the project - see 4cb96a4. I think that makes this PR redundant and I'm going to close it. Thanks again for your time and I hope it's some consolation that you've encouraged us to fix this!

@floehopper floehopper closed this Sep 8, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment