-
Notifications
You must be signed in to change notification settings - Fork 479
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 dependency constraint on ruby-git #1436
Conversation
Cool, yeah, looks reasonable to me |
The test fail looks like it would have been caused by this update |
Let me check the error 👀 Update: It looks like danger with https://github.com/ruby-git/ruby-git/releases/tag/v1.15.0 I will try to fix the failure in this PR. Update2: The new feature added to
|
@@ -43,7 +43,7 @@ | |||
@dm = testing_dangerfile | |||
expect do | |||
@dm.env.scm.diff_for_folder(dir + "/subdir") | |||
end.to raise_error(ArgumentError, /path does not exist/) | |||
end.to raise_error(ArgumentError, /is not the top level git directory/) |
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.
Previously ArgumentError
was caused by Git.open
called at git_repo.rb
L20 when the path isn't a top-level git directory. But from git
gem v1.15.0, Git.open
won't cause the error any more.
My change maintains the current intention to keep the backward compatibility but gives a better error message to educate users.
end | ||
end | ||
repo = Git.open git_top_level | ||
git_top_level = find_git_top_level_if_needed!(folder, lookup_top_level) |
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 extracted logic to raise an error to keep the compatibility, but the behaviour should be the same as before, apart from the error message.
end | ||
end | ||
|
||
it "looks up the top level git folder when requested" do | ||
with_git_repo do |dir| | ||
@dm = testing_dangerfile | ||
@dm.env.scm.diff_for_folder(dir + "/subdir", lookup_top_level: true) |
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 previous spec didn't assert anything. I made the expectation explicit.
PR is ready to review now. I confirmed RSpec passed all specs with |
@manicmaniac hey! could you, please, like release a new version or something? 🙂 |
Sorry I don't have the right to publish new release. @orta can do it. |
Shipped, thanks @manicmaniac ! |
Thanks to @ainame from me 🥇 |
In this PR #1419, the gemspec for runtime dependency on
git
was bumped to~> 1.13.0
.When updating Danger to 9.3.0 today, I noticed the dependency-resolving issue. In my project, bundler has already fixed
git
at version1.18.0
, so installing danger v9.3.0 means we need to downgradegit
. Bundler didn't allows me to do that by default. (I needed to delete Gemfile.lock and re-run bundle install)This is what we got when updating
danger
via Renovate.The previous constraint of being flexible on the minor versions is easier for anyone who has been using Danger.
According to the PR author, there was no reason to limit the version constraint to the patch version.
#1419 (comment)
cc: @manicmaniac