-
-
Notifications
You must be signed in to change notification settings - Fork 327
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
Remove mime-types
, rexml
, and webrick
runtime dependencies
#559
Conversation
add_gem 'sqlite3', '~> 1.4' | ||
add_gem 'selenium-webdriver', '~> 4', group: :test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The selenium-webdriver
uses the rexml
library. Downstream projects only need rexml
if they are also using the optional Selenium framework.
The selenium-webdriver
maintainers have added an explicit runtime dependency in version 4. This resolves the test errors when running on Ruby 3.
mime-types
and rexml
runtime dependenciesmime-types
, rexml
, and webrick
runtime dependencies
step 'I append to "Gemfile" with:', "gem 'webrick', group: 'test'\n" | ||
run_command_and_stop('bundle install --jobs 4') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Install the webrick
gem in this step to support the Capybara configuration above.
Capybara.server = :webrick |
This configuration is optional for downstream projects. If developers choose to use webrick
, they can add the gem as a dependency in their Rails project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of stylistic changes to make. All in all I've not got any major problems with it.
Would be good to get your take on the ruby3 gems. I'm on the fence and didn't introduce it originally.
add_gem 'sqlite3', '~> 1.4' | ||
add_gem 'selenium-webdriver', '~> 4', group: :test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stylistically I'm not a fan of single digits in the twiddle. I know it works, but for completeness / style can we say 4.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 10649a3
@@ -65,6 +65,8 @@ | |||
} | |||
|
|||
step 'I append to "features/support/env.rb" with:', selenium_config | |||
step 'I append to "Gemfile" with:', "gem 'webrick', group: 'test'\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These references to step
need removing actually. So I'd prefer not doing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 7928281
s.add_runtime_dependency('nokogiri', '~> 1.10') | ||
s.add_runtime_dependency('railties', ['>= 5.0', '< 8']) | ||
s.add_runtime_dependency('rexml', '~> 3.0') # rexml is a bundled gem from ruby 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still support lower ruby versions. I'm kinda on the fence about this, as ideally people should be controlling their own version constraints if they need this. So I think I'm 50-50 about this. I don't see the harm in retaining this until we're ruby 3.0+ only (Which isn't likely to be too long at current development rates).
That being said, if you're passionate about this, I don't see the harm in making this change. It was originally made when we introduced ruby3 into our support matrix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My argument is there is no code in the cucumber-rails
gem that uses rexml
or webrick
. It operates identically, with or without these dependencies, on all supported versions of Ruby. There is no value in declaring them as runtime dependencies.
@@ -4,7 +4,7 @@ | |||
require 'cucumber' | |||
require 'capybara' | |||
|
|||
module CucumberRailsHelper | |||
module CucumberRailsHelper # rubocop:todo Layout/ModuleLength |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wherever possible we're trying to avoid doing in-line exclusions and using the top level control with a comment explaining why.
Ideally longer term we should compress this down a bit. But it is test only code, so it's low prio.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed this by removing a superfluous line in 7f667a8
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think this is good. I do think some of the tidy ups inside the module do need cleaning up more. But that can be done at another time.
Hi @orien, 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:
On behalf of the Cucumber core team, |
Summary
I notice
cucumber-rails
doesn't directly use themime-types
,rexml
, andwebrick
gems.Let's remove them from the gemspec to avoid forcing unnecessary dependencies on downstream projects.
How Has This Been Tested?
The automated test suite passes showing these gems are unneeded.
Types of changes
Checklist: