fix: retrieve active pull requests against chosen target branch #728
Conversation
|
What happens if the branch is not set in the options/configuration? |
|
When creating a new instance of Dependabot::Source the branch property is also passed in directly. You could choose to get the default branch as fallback and apply that everywhere, making it completely optional. |
|
@mburumaxwell |
THETCR
force-pushed
the
fix/active-pull-requests
branch
from
59a3137 to
7aa6c7e
Compare
|
Wouldn't it be better to create a new variable named |
| @@ -554,8 +554,8 @@ def show_diff(original_file, updated_file) | |||
| credentials: $options[:credentials] | |||
| ) | |||
| user_id = azure_client.get_user_id | |||
| default_branch_name = azure_client.fetch_default_branch($source.repo) | |||
| active_pull_requests = azure_client.pull_requests_active(user_id, default_branch_name) | |||
| $options[:branch] = azure_client.fetch_default_branch($source.repo) unless $options[:branch] | |||
There was a problem hiding this comment.
- $options[:branch] = azure_client.fetch_default_branch($source.repo) unless $options[:branch]
+ target_branch = $options[:branch] || azure_client.fetch_default_branch($source.repo)There was a problem hiding this comment.
Creating a local variable and so creating a new reference in memory seems like a waste of resources, since you already have a pointer that was intended for this purpose.
After execution the branch property is not going to be changed by the end user anymore and there is no further mutation being done on target_branch, so I reckon this unnecessarily costs memory and CPU cycles.
There was a problem hiding this comment.
@mburumaxwell any feedback on the above?
There was a problem hiding this comment.
The preference is to not do any changes to the $options object after the initial setup.
| default_branch_name = azure_client.fetch_default_branch($source.repo) | ||
| active_pull_requests = azure_client.pull_requests_active(user_id, default_branch_name) | ||
| $options[:branch] = azure_client.fetch_default_branch($source.repo) unless $options[:branch] | ||
| active_pull_requests = azure_client.pull_requests_active(user_id, $options[:branch]) |
There was a problem hiding this comment.
- active_pull_requests = azure_client.pull_requests_active(user_id, $options[:branch])
+ active_pull_requests = azure_client.pull_requests_active(user_id, target_branch)| @@ -922,7 +922,7 @@ def show_diff(original_file, updated_file) | |||
| # look for pull requests that are no longer needed to be abandoned | |||
| if $options[:close_unwanted] | |||
| puts "Looking for pull requests that are no longer needed." | |||
| active_pull_requests = azure_client.pull_requests_active(user_id, default_branch_name) | |||
| active_pull_requests = azure_client.pull_requests_active(user_id, $options[:branch]) | |||
There was a problem hiding this comment.
- active_pull_requests = azure_client.pull_requests_active(user_id, $options[:branch])
+ active_pull_requests = azure_client.pull_requests_active(user_id, target_branch)
|
This should be fixed in #797 |
|
Released yesterday. You may create a new issue if you experience trouble. Closing this |
With the current behaviour PRs are not updated or closed when the target branch deviates from the default branch.