Allow leading v on commit message versions #245
Conversation
jonmcquillan
changed the title
There was a problem hiding this comment.
This looks great, nice job with the tests.
Just need the one behavior change and then happy to merge... looks pretty straightforward.
Sorry took so long to get this reviewed, but now that I'm familiar with what you're trying to do I can review any updates quickly.
| expect(updatedDependencies[0].packageEcosystem).toEqual('nuget') | ||
| expect(updatedDependencies[0].targetBranch).toEqual('main') | ||
| expect(updatedDependencies[0].prevVersion).toEqual('4.0.1') | ||
| expect(updatedDependencies[0].newVersion).toEqual('4.2.2') |
There was a problem hiding this comment.
As I explained in #244 (comment), I think it'd be more consistent if this was v4.2.2 preserving the "v"...
src/dependabot/update_metadata.ts
Outdated
| const prev = bumpFragment?.groups?.from ?? '' | ||
| const next = bumpFragment?.groups?.to ?? '' | ||
| const prev = bumpFragment?.groups?.from.replace('v', '') ?? '' | ||
| const next = bumpFragment?.groups?.to.replace('v', '') ?? '' |
There was a problem hiding this comment.
I think if you move this replace() from here to where the update is actually calculated then it'll preserve the "v"
src/main.test.ts
Outdated
| @@ -132,6 +132,75 @@ test('it sets the updated dependency as an output for subsequent actions', async | |||
| expect(core.setOutput).toBeCalledWith('cvss', 0) | |||
| }) | |||
|
|
|||
| test('it sets the updated dependency as an output for subsequent actions when there is a leading v in the commit message version', async () => { | |||
There was a problem hiding this comment.
This is exactly why we should preserve the leading "v"... the user may do post-processing and I'd rather leave the decision to "strip" any prefix up to them...
Fix #244