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

Logic error when checking for symbolized keys #3

Merged
merged 1 commit into from Jul 29, 2016
Merged

Conversation

kwando
Copy link
Contributor

@kwando kwando commented Jul 29, 2016

Current code was always symbolizing keys

Current code was always symbolizing keys
@odlp
Copy link
Owner

odlp commented Jul 29, 2016

Hi @kwando I'm not sure this is a error - looking at the implementation of #keys_are_symbols?
https://github.com/odlp/simplify_rb/blob/master/lib/simplify_rb.rb#L103

This #keys_are_symbols? method expects an array of keys, and uses all? within itself.

My intention was to always deal with keys as symbols, to reduce memory footprint. If some keys are symbols, it symbolizes them (this is the current behaviour). If all the keys are already symbols, it carries on.

Would you like a mode to keep the keys as strings? If so we could add an option for this.

@kwando
Copy link
Contributor Author

kwando commented Jul 29, 2016

Your code is wrong since, try stepping through a debugger and you will see ;)

The keys_are_symbols? method is called once with an argument looking like this [[:x, :y], [:x, :y]] and [[:x, :y], [:x, :y]].all?{|k| k.is_a?(Symbol)} is always false..

@odlp
Copy link
Owner

odlp commented Jul 29, 2016

Ah I see! Thank you for the explanation 👍

I was misinterpreting the original code as sending a flattened array (e.g. [:x, :y, :x, :y]) rather than a nested array like [[:x, :y], [:x, :y]].

Thanks again. I will try and make a new release shortly.

@odlp odlp merged commit 0b8cb94 into odlp:master Jul 29, 2016
@kwando
Copy link
Contributor Author

kwando commented Jul 29, 2016

No problemo, sorry if I came across as rude or something. Was not intentional =)

@odlp
Copy link
Owner

odlp commented Jul 29, 2016

Not at all, I was just confused! I just pushed version 0.1.3 to Rubygems with your fix included.

Thanks.

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