-
Notifications
You must be signed in to change notification settings - Fork 229
Get to green, then update dependencies #526
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -71,7 +71,7 @@ def run_task | |||||
|
|
||||||
| def print_starting_up_output | ||||||
| puts "\n\n!!! Running `#{name}` rake command\n" | ||||||
| puts "!!! Inspecting #{paths} #{options.empty? ? '' : "with options #{options}"}\n\n" | ||||||
| puts "!!! Inspecting #{paths} #{"with options #{options}" unless options.empty?}\n\n" | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @faisal maybe changing this to single quotes helps the reading? what do you think?
Suggested change
|
||||||
| end | ||||||
|
|
||||||
| def options_as_arguments | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,30 +35,30 @@ Gem::Specification.new do |spec| | |
| spec.add_dependency 'launchy', '>= 2.5.2' | ||
| spec.add_dependency 'parser', '>= 3.3.0.5' | ||
| spec.add_dependency 'rainbow', '~> 3.1.1' | ||
| spec.add_dependency 'reek', '~> 6.4.0', '< 7.0' | ||
| spec.add_dependency 'reek', '~> 6.5.0', '< 7.0' | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the most significant change (not a dev dependency): https://github.com/troessner/reek/blob/master/CHANGELOG.md But based on the reek changelog it should be fine to merge. |
||
| spec.add_dependency 'rexml' | ||
| spec.add_dependency 'ruby_parser', '~> 3.21' | ||
| spec.add_dependency 'simplecov', '>= 0.22.0' | ||
| spec.add_dependency 'tty-which', '~> 0.5.0' | ||
| spec.add_dependency 'virtus', '~> 2.0' | ||
|
|
||
| spec.add_development_dependency 'aruba', '~> 2.3.0' | ||
| spec.add_development_dependency 'aruba', '~> 2.3.1', '>= 2.3.1' | ||
| spec.add_development_dependency 'bundler', '>= 2.0.0' | ||
| if RUBY_PLATFORM == 'java' | ||
| spec.add_development_dependency 'pry-debugger-jruby' | ||
| else | ||
| spec.add_development_dependency 'byebug', '~> 11.0', '>= 10.0' | ||
| spec.add_development_dependency 'byebug', '~> 12.0', '>= 10.0' | ||
| end | ||
| spec.add_development_dependency 'cucumber', '~> 9.2.1', '!= 9.0.0' | ||
| spec.add_development_dependency 'cucumber', '~> 10.0.0', '!= 9.0.0' | ||
| spec.add_development_dependency 'diff-lcs', '~> 1.3' | ||
| spec.add_development_dependency 'fakefs', '~> 2.6.0' | ||
| spec.add_development_dependency 'fakefs', '~> 3.0.0' | ||
| spec.add_development_dependency 'irb' | ||
| spec.add_development_dependency 'mdl', '~> 0.13.0', '>= 0.12.0' | ||
| spec.add_development_dependency 'minitest', '~> 5.25.2', '>= 5.3.0' | ||
| spec.add_development_dependency 'minitest-around', '~> 0.5.0', '>= 0.4.0' | ||
| spec.add_development_dependency 'mocha', '~> 2.7.1' | ||
| spec.add_development_dependency 'ostruct' | ||
| spec.add_development_dependency 'rake', '~> 13.2.0', '>= 11.0.0' | ||
| spec.add_development_dependency 'rake', '~> 13.3.0', '>= 11.0.0' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @faisal one more question: why did you bump the dependencies in the same PR? Was that needed to make CI pass?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If not, could we split it? Sorry for asking a lot but want to make sure we are not blowing it up
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR wound up collapsing a bunch of other PRs: #504, #512, #524. In particular, FakeFS < 3 absolutely spewed the console with |
||
| spec.add_development_dependency 'rdoc' | ||
| spec.add_development_dependency 'rexml', '>= 3.2.0' | ||
| spec.add_development_dependency 'rubocop', '>= 1.72.0', '< 2.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.
@faisal One question, how long do you think we should have this monkey patch? Is there any way to have a breaking condition to remember us removing it?
Like onces Cucumber is at version X, raise "we do not need this patch please remove me"
Just wondering 🤔 to don't have unnecessary code in the future
Uh oh!
There was an error while loading. Please reload this page.
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 wrestled with this question. The initial version of the patch did a version check on Cucumber as well as Ruby, but tests then failed when they updated Cucumber without fixing the issue.
I think our options are:
In either case, if we merge this I think we should open an issue to revert the workaround at such time when it becomes unnecessary.
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'm watching for cucumber/cucumber-ruby-core#292 to land, or to be closed out in favor of something equivalent.
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 see. Yeah, I agreed, let's keep it as it is and wait for cucumber/cucumber-ruby-core#292 to be merged, in good news, there is already a PR.
JFI: I'm trying to get someone with powers to merge this ASAP
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.
One last option would be to rework or replace that feature so as to not call cucumber-core. That seems more involved than is warranted.
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.
Yeah! that is another great option. do you have an estimation on which tests are using that feature? How long would it take?
I mean, either way we can begin with merging this PR then we can stop using that feature and upgrade cucumber as needed.