Refactored url.rb to be compliant with Ruby Style Guide. #3544

Merged
merged 2 commits into from Mar 4, 2015

Conversation

Projects
None yet
5 participants
@MartinRogalla
Contributor

MartinRogalla commented Mar 4, 2015

  • Single Quotes
  • Fixed typo in variable name.
  • Removed Redundant Escaping in Regular Expression.

Signed-off-by: Martin Jorn Rogalla martin@martinrogalla.com

Refactored url.rb to compliant with Ruby Style Guide.
 - Single Quotes
 - Fixed Typo's in variable names.
 - Removed Redundant Escaping in Regular Expressions.

Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 4, 2015

Member

To which ruby style guide do you refer?

Member

parkr commented Mar 4, 2015

To which ruby style guide do you refer?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 4, 2015

Contributor

Make it all double quotes if you're gonna do anything about quotes because we've all strong opinions on code style. So the quotes part gotta go IMO because I don't care what the Ruby Style Guide has to say about quotes at all.

The rest is fine IMO.

Contributor

envygeeks commented Mar 4, 2015

Make it all double quotes if you're gonna do anything about quotes because we've all strong opinions on code style. So the quotes part gotta go IMO because I don't care what the Ruby Style Guide has to say about quotes at all.

The rest is fine IMO.

@MartinRogalla

This comment has been minimized.

Show comment
Hide comment
@MartinRogalla

MartinRogalla Mar 4, 2015

Contributor

@parkr I believe I might have had the wrong style guide. https://github.com/bbatsov/ruby-style-guide#strings

So what exactly should be within single quotes and what should be in double quotes?

Contributor

MartinRogalla commented Mar 4, 2015

@parkr I believe I might have had the wrong style guide. https://github.com/bbatsov/ruby-style-guide#strings

So what exactly should be within single quotes and what should be in double quotes?

@willnorris

This comment has been minimized.

Show comment
Hide comment
@willnorris

willnorris Mar 4, 2015

Contributor

Jekyll follows GitHub's ruby style guide: https://github.com/styleguide/ruby (which is based on bbatsov/ruby-style-guide, but differs in some places)

Contributor

willnorris commented Mar 4, 2015

Jekyll follows GitHub's ruby style guide: https://github.com/styleguide/ruby (which is based on bbatsov/ruby-style-guide, but differs in some places)

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 4, 2015

Contributor

IMO leave quotes well alone unless you are directly working on the code itself, there is no reason to enforce one type of quote if people prefer other types because of muscle memory. This isn't C/++. I always personally use double quotes for the same reason as Github and only use Single quotes when it enhances the readability of the source 'this is an "example"' is an example of when I would use single quotes. However, my preference aside, I'm not about to go and tell somebody to use one or the other because that's something that can be fixed later when the code is directly worked on indirect of the actual fix happening.

Contributor

envygeeks commented Mar 4, 2015

IMO leave quotes well alone unless you are directly working on the code itself, there is no reason to enforce one type of quote if people prefer other types because of muscle memory. This isn't C/++. I always personally use double quotes for the same reason as Github and only use Single quotes when it enhances the readability of the source 'this is an "example"' is an example of when I would use single quotes. However, my preference aside, I'm not about to go and tell somebody to use one or the other because that's something that can be fixed later when the code is directly worked on indirect of the actual fix happening.

@MartinRogalla

This comment has been minimized.

Show comment
Hide comment
@MartinRogalla

MartinRogalla Mar 4, 2015

Contributor

@envygeeks Okay, I won't change the quotes which are already in place, but don't really understand the following line as it seems inconsistent to me:

# Append a trailing slash to the URL if the unsanitized URL had one
url << "/" if in_url[-1].eql?('/')

I won't change any of the quotes which are in place and use double quotes wherever possible.

Contributor

MartinRogalla commented Mar 4, 2015

@envygeeks Okay, I won't change the quotes which are already in place, but don't really understand the following line as it seems inconsistent to me:

# Append a trailing slash to the URL if the unsanitized URL had one
url << "/" if in_url[-1].eql?('/')

I won't change any of the quotes which are in place and use double quotes wherever possible.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 4, 2015

Contributor

@MartinRogalla Yeah that line can definitely change if you want to change it!

Contributor

envygeeks commented Mar 4, 2015

@MartinRogalla Yeah that line can definitely change if you want to change it!

@willnorris

This comment has been minimized.

Show comment
Hide comment
@willnorris

willnorris Mar 4, 2015

Contributor
# Append a trailing slash to the URL if the unsanitized URL had one
url << "/" if in_url[-1].eql?('/')

We've switched to using end_with? elsewhere, which would be a lot easier to read than this

Contributor

willnorris commented Mar 4, 2015

# Append a trailing slash to the URL if the unsanitized URL had one
url << "/" if in_url[-1].eql?('/')

We've switched to using end_with? elsewhere, which would be a lot easier to read than this

Corrected quote-usage. Replaced [-1].eql with end_with.
Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
@MartinRogalla

This comment has been minimized.

Show comment
Hide comment
@MartinRogalla

MartinRogalla Mar 4, 2015

Contributor

@willnorris thanks for the suggestion! Should be fixed.

Contributor

MartinRogalla commented Mar 4, 2015

@willnorris thanks for the suggestion! Should be fixed.

parkr added a commit that referenced this pull request Mar 4, 2015

Merge pull request #3544 from delftswa2014/micro_refactor_url
Refactored url.rb to be compliant with Ruby Style Guide.

@parkr parkr merged commit ced9146 into jekyll:master Mar 4, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

parkr added a commit that referenced this pull request Mar 4, 2015

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 4, 2015

Member

Thanks!!

Member

parkr commented Mar 4, 2015

Thanks!!

@MartinRogalla MartinRogalla deleted the delftswa2014:micro_refactor_url branch Mar 9, 2015

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

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