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

Upgrade cucumber-gherkin to v10 #221

Merged
merged 2 commits into from
Mar 2, 2020

Conversation

trammel
Copy link

@trammel trammel commented Feb 27, 2020

Gherkin has changed it's name to cucumber-gherkin ( https://rubygems.org/gems/cucumber-gherkin ), so this bumps the version and uses the new name.

Fixes to the expected metadata from Gherkin. Tags & Examples are now optional, so .blank? is used to test, where .empty? breaks on nil

Removed link to rubyforge_project in the gemspec, because it's deprecated.

🍐 @jayzz55

Gherkin has changed it's name to cucumber-gherkin ( https://rubygems.org/gems/cucumber-gherkin ), so this bumps the version and uses the new name.

Fixes to the expected metadata from Gherkin. Tags & Examples are now optional, so .blank? is used to test, where .empty? breaks on nil.

Removed link to rubyforge_project in the gemspec, because it's deprecated.

:pear: @jayzz55
@lesleh
Copy link

lesleh commented Feb 28, 2020

Is blank? the right method to use here? It's not part of Ruby, it comes from Rails/ActiveSupport.

The project has a requirement of ruby >= 2.3 so you can use the safe navigation operator instead:

thing&.empty?

It looks like activesupport will be a transient dependency for protobuf cucumber/common@c208362#diff-9e6be4c16acf7abd5846c3eefb389ca2R22 so using .blank? while convenient is probably not a good long term option.

Refactored to remove .blank?
@trammel
Copy link
Author

trammel commented Mar 1, 2020

Is blank? the right method to use here? It's not part of Ruby, it comes from Rails/ActiveSupport.

The project has a requirement of ruby >= 2.3 so you can use the safe navigation operator instead:

Newer versions of 'cucumber-messages' (https://github.com/cucumber/cucumber/tree/master/messages/ruby the replacement for 'messages') require 'protobuf-cucumber' ( https://github.com/ruby-protobuf/protobuf ) , which is a pure ruby replacement for the 'google-protobuf' gem, which is an improvement in one way, because 'google-protobuf' had a bad habit of causing segfaults on alpine docker images. And 'protobuf-cucumber' requires 'activesupport'. However, it looks like it'll be a transient dependency, cucumber/common@c208362#diff-9e6be4c16acf7abd5846c3eefb389ca2R22

I've refactored the usage of .blank? to use alternatives. Thanks for the review.

@gongo
Copy link
Collaborator

gongo commented Mar 2, 2020

@trammel @lesleh Thanks!! 🍕

@gongo gongo merged commit 8283d09 into jnicklas:master Mar 2, 2020
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.

4 participants