Skip to content

Conversation

@Eomm
Copy link
Member

@Eomm Eomm commented Dec 7, 2021

fixes #124

It should support submodules as well (the #98 PR adds the any option to skip this check)

@Eomm Eomm requested a review from simoneb December 7, 2021 16:28
Copy link
Collaborator

@simoneb simoneb left a 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?

@Eomm
Copy link
Member Author

Eomm commented Dec 8, 2021

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.
I think the behaviour is consistent within the documentation now

t.ok(stubs.fetchStub.calledOnce)
})

tap.test('should check submodules semver when target is set', async t => {
Copy link
Collaborator

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

Copy link
Collaborator

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

Copy link
Member Author

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.

Copy link
Collaborator

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

Copy link
Member Author

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:

sitiom/dotfiles#1

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Eomm Eomm merged commit 8695e84 into main Dec 8, 2021
@Eomm Eomm deleted the semver-like branch December 8, 2021 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Manage semver-like formats

3 participants