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

Code style improvements #661

Merged
merged 1 commit into from Aug 25, 2017
Merged

Conversation

Darhazer
Copy link
Contributor

This mostly converts hashes to 1.9 style (following #647) and also makes some minor code style corrections (in a separate commit)

Copy link
Owner

@rodjek rodjek left a comment

Choose a reason for hiding this comment

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

It's going to be hard to break the muscle memory of using 1.8 style hashes, but I can't really argue with the change :)

A couple of changes I'd like to see in regards to the proposed changes from Hash#include? to Array#include? though

@@ -81,7 +81,7 @@ def title_tokens
}
title_array_tokens = tokens[(array_start_idx + 1)..(token_idx - 2)]
result += title_array_tokens.select { |token|
{:STRING => true, :NAME => true}.include? token.type
[:STRING, :NAME].include? token.type
Copy link
Owner

Choose a reason for hiding this comment

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

The reason I went with hashes here is because Array#include? is O(n) and Hash#include? is O(1). If we're going to clean this up, I'd like to see this become a Set instead of an Array so that we don't slow things down.

@@ -116,16 +116,16 @@ def resource_indexes
real_idx = token_idx + idx + 1
if tokens[real_idx].type == :LBRACE
depth += 1
elsif {:SEMIC => true, :RBRACE => true}.include? tokens[real_idx].type
elsif [:SEMIC, :RBRACE].include? tokens[real_idx].type
Copy link
Owner

Choose a reason for hiding this comment

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

The reason I went with hashes here is because Array#include? is O(n) and Hash#include? is O(1). If we're going to clean this up, I'd like to see this become a Set instead of an Array so that we don't slow things down.

@Darhazer
Copy link
Contributor Author

Thanks for noting the performance consideration.
Returned array#include? back to hash#include?, just with the new style

@rodjek
Copy link
Owner

rodjek commented Aug 25, 2017

I'm going to rebase this and remove the hash syntax updates, as currently we need to maintain Ruby 1.8.7 support in 2.x as it hasn't officially been deprecated. We'll come back to the hash syntax changes later and drop ruby 1.8.7 support during the major version bump to 3.x.

@rodjek rodjek merged commit 1ec2a67 into rodjek:master Aug 25, 2017
@rodjek rodjek added this to the Next release (2.4.0 maybe?) milestone Aug 25, 2017
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

2 participants