Skip to content
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

Expose svu functionality as a library and internal refactoring #126

Merged
merged 2 commits into from
Sep 19, 2023

Conversation

ahaasler
Copy link
Contributor

@ahaasler ahaasler commented Sep 1, 2023

This PR addresses the need to use svu as a go library directly in code (partially addresses #123) and introduces internal refactoring for maintainability.

I've exposed the public API in pkg/svu, allowing developers to use svu as an import name directly in their code as github.com/caarlos0/svu/pkg/svu.

The exposed API uses a variadic options pattern, which mitigates the risk of future interface breaking changes. This approach ensures easy maintenance through adapter functions and flexibility for internal/svu to implement changes and new features without being restricted with the API contract.

I had to move some functionality from the main package to internal/svu. This does introduce a dependency on internal/git in internal/svu that can be resolved if needed, maybe moving that code to pkg/svu instead?

I've defined constants with command names and tag-mode options to prevent mismatches. However, this change has not been extended to the CLI yet, just in case it's not desired.

Open PR Conflict:

I've noticed an open PR (#125) that conflicts with these changes. I'm open to adapting my code to resolve any conflicts that may arise once that PR is merged. Alternatively, I'm willing to assist in adapting the code in #125 to ensure a smoother merge process if this PR is merged before.

I welcome any input, adjustments, or recommendations from both the community and maintainers to improve this solution and maximize its usefulness for svu users.

@caarlos0
Copy link
Owner

hey, thanks for the PR @ahaasler

I merged #125 as it seemed easier to integrate that pr here than the other way around.

Other than that, on first look, your PR looks good!
Once we fixed the conflicts I think we can merge it 🙏🏻

@ahaasler
Copy link
Contributor Author

Hi, I have resolved the conflicts and checked that PR #125 works as expected. During testing I found out that there's a bug in #125 or that the README.md is incorrect:

The docs say:

$ svu current
v1.2.3-alpha.2+123

$ svu prerelease
v1.2.3-alpha.3

$ svu prerelease --pre-release alpha.33 --build 243
v1.2.3-alpha.33+243

But I got different result from both the code in the main branch and with my changes applied:

$ go install github.com/caarlos0/svu@main
go: downloading github.com/caarlos0/svu v1.11.1-0.20230912125155-881d9c3a6e5d
$ svu --version
svu version dev
module version: v1.11.1-0.20230912125155-881d9c3a6e5d, checksum: h1:g7aZc5Zh0HxpTKRVbJvl2o7aFI0ZSt7Gcybw3T4rhRY=

$ git tag
v1.2.3-alpha.2+123
$ git log
commit 579ce447d1589ba0bd37cfaa30ca14f4067a3690 (HEAD -> main, tag: v1.2.3-alpha.2+123)
Author: Adrian Haasler García <dev@ahaasler.com>
Date:   Tue Sep 12 20:54:44 2023 +0200

    feat: add a

$ svu current
v1.2.3-alpha.2+123
$ svu prerelease
v1.2.3-alpha.3
$ svu prerelease --pre-release alpha.33 --build 243
v1.2.3-alpha.34+243

$ ~/workspace/svu/svu current
v1.2.3-alpha.2+123
$ ~/workspace/svu/svu prerelease
v1.2.3-alpha.3
$ ~/workspace/svu/svu prerelease --pre-release alpha.33 --build 243
v1.2.3-alpha.34+243

Notice the v1.2.3-alpha.34+243 instead of v1.2.3-alpha.33+243

@caarlos0
Copy link
Owner

hmm, looks like a bug indeed

@bradleyjones do you want to take a look?

@bradleyjones
Copy link
Contributor

Fix for that issue here #134

@ahaasler
Copy link
Contributor Author

Hi, I have resolved the conflicts to include #134.

@caarlos0
Copy link
Owner

awesome, thank you!

@caarlos0 caarlos0 merged commit 337a8f2 into caarlos0:main Sep 19, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants