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

Fix for issue: #1199: Changed DataTable#symbolic_hashes to not change the state of the table #1200

Conversation

Ben-Behar
Copy link
Contributor

Summary

DataTable#symbolic_hashes will no longer set the instance variable @header_conversion_proc but instead will act in a similar manner to DataTable#hashes except returning a new array of hashes with symbolic keys.

Details

Changed DataTable#symbolic_hashes to call DataTable#hashes and then map the return value to a new hash with all keys changed to symbols using the method DataTable#symbolize_key(key).

Motivation and Context

Solves the issue reported here #1199

How Has This Been Tested?

Added the following test: spec/cucumber/multiline_argument/data_table_spec.rb. This is intended to ensure that after using DataTable#symbolic_hashes, no error is raised when using DataTable#hashes (see issue: #1199 )

The test only been run in the following environments so far:

  • Ruby 2.3.0 using bundle exec rake

Types of changes

  • Refactor (code change that does not change external functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I've added tests for my code
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@danascheider
Copy link
Contributor

Thanks @Ben-Behar! I'll take a look at this and see what's happening with these failing tests. :)

@danascheider
Copy link
Contributor

So this is an issue with JRuby we've been having in Travis as a result of a new JRuby release. There's a fix on master - would you mind rebasing @Ben-Behar?

@Ben-Behar Ben-Behar force-pushed the 1199-DataTable-avoid-state-change-in-symbolic-hashes branch from ee2bb9a to 0b5498b Compare September 20, 2017 01:04
@Ben-Behar
Copy link
Contributor Author

Rebase is done. Hopefully my git push --force won't mess up any building/testing. 🤞

@danascheider
Copy link
Contributor

Thanks! I'm checking out the appveyor failure but I'm not sure it's because of your code so we may be able to merge anyway.

@@ -159,8 +159,10 @@ def hashes
# [{:foo => '2', :bar => '3', :foo_bar => '5'}, {:foo => '7', :bar => '9', :foo_bar => '16'}]
#
def symbolic_hashes
@header_conversion_proc = lambda { |h| symbolize_key(h) }
@symbolic_hashes ||= build_hashes
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this build_hashes method used elsewhere? If not I'd delete it. Same with the @header_conversion_proc - make sure it isn't used elsewhere before deleting it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • build_hashes is called above on line 147 within the method hashes, which in turn is called on line 163.
  • @header_conversion_proc is used elsewhere and the modification to this variable in @symbolic_hashes is what is/was causing the issue.

Personally, I wouldn't mind removing usage of @header_conversion_proc throughout the file as it seems a bit unnecessary. However, I think that is outside the scope of this bugfix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, thanks for clarifying!

@danascheider
Copy link
Contributor

It looks like the Appveyor failures are the same ones we already have on master, so I'm going to go ahead and merge this.

@danascheider danascheider merged commit 77e7120 into cucumber:master Sep 21, 2017
@aslakhellesoy
Copy link
Contributor

Hi @Ben-Behar,

Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾

In return for this generous offer we hope you will:

  • ✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
  • 💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
  • 💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
  • ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
  • 🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber

@lock
Copy link

lock bot commented Oct 24, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 24, 2018
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