-
Notifications
You must be signed in to change notification settings - Fork 35
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: semver like titles #125
Conversation
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.
Do we have a test which checks that even when specifying a target, submodules are handled? I'm not actually sure what would be the expected behavior though, maybe it's worth deciding and documenting?
Added a new test to check it. |
@@ -263,3 +263,29 @@ tap.test('should call external api for github-action-merge-dependabot major rele | |||
t.ok(stubs.logStub.logWarning.calledOnce) | |||
t.ok(stubs.fetchStub.calledOnce) | |||
}) | |||
|
|||
tap.test('should check submodules semver when target is set', async t => { |
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 not what submodules look like. submodules have a commit sha in the pr title (iirc) #95
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.
@Eomm lmk if the comment is clear. It's about the title of the PR you have in this test not matching what happens when a version of a submodule tries to be bumped. You only get a SHA in that case
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, I'm on it. Let me push the wip code
I'm trying to ignore the case:
- the PR title is not a semver and the user set the
target
value
this check is a bit tricky due the prerelease
stuff and the coercion.
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 don't have to support everything, it would be a good step if we didn't break anything of the existing and we document the behavior for edge cases so we know what we can work on next
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 have implemented the most simple check, using this PR as reference:
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.
lgtm
fixes #124
It should support submodules as well (the #98 PR adds the
any
option to skip this check)