-
Notifications
You must be signed in to change notification settings - Fork 496
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
Branch selection #1469
Branch selection #1469
Conversation
… name to update branch name
Codecov Report
@@ Coverage Diff @@
## master #1469 +/- ##
==========================================
- Coverage 70.26% 69.58% -0.69%
==========================================
Files 113 113
Lines 1786 1815 +29
Branches 51 49 -2
==========================================
+ Hits 1255 1263 +8
- Misses 531 552 +21
Continue to review full report at Codecov.
|
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.
Only comments for the moment.
If you now get 2 PRs for the same dependency update in different branches. I t would make sense to mention the branch that gets updated if it is not the default branch.
- Update scalafmt-core to 2.5.2
- Update scalafmt-core to 2.5.2 (maintenance)
modules/core/src/main/scala/org/scalasteward/core/vcs/data/Repo.scala
Outdated
Show resolved
Hide resolved
modules/core/src/main/scala/org/scalasteward/core/git/package.scala
Outdated
Show resolved
Hide resolved
seems like I adjusted the PR to all the suggestions (correct me if i'm wrong though :)) |
@fthomas do you perhaps have some suggestions regarding this PR? |
def branchFor(update: Update): Branch = | ||
Branch(s"update/${update.name}-${update.nextVersion}") | ||
def branchFor(update: Update, baseBranch: Branch): Branch = | ||
Branch(s"update/${baseBranch.name}/${update.name}-${update.nextVersion}") |
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.
branchFor
is used in vcs.listingBranch
to create the head
parameter for VCSApiAlg#listPullRequests
which is used to find already existing Scala Steward PRs. If we change this for PRs that target the default branch, Scala Steward will not find older PRs it already created and will create new ones. I guess this could result in thousands new but duplicated PRs by @scala-steward.
Can we make this optional and only pass it if baseBranch
is not the default branch?
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.
sure! i'll push a fix soon:)
|
||
def commitMsgFor(update: Update, commitsConfig: CommitsConfig): String = { | ||
def commitMsgFor(update: Update, commitsConfig: CommitsConfig, branch: Branch): String = { |
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.
This is only stylistic, but can we mention the branch only if it is not the default?
commitMsgFor - mention the branch only if it is not default
…teward into branch-selection � Conflicts: � modules/core/src/main/scala/org/scalasteward/core/nurture/NurtureAlg.scala � modules/core/src/test/scala/org/scalasteward/core/vcs/data/NewPullRequestDataTest.scala
@fthomas I applied your suggestions - let me know if I missed something important 👍 |
What blocks this PR from being merged? |
This is a useful feature for us. |
Also waiting on this PR :D |
Closes #20