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

Configurable version increments per Type #164

Closed
wants to merge 1 commit into from

Conversation

ccopsey
Copy link
Contributor

@ccopsey ccopsey commented Dec 14, 2023

I expect that this PR will be declined as it takes a slightly different view of the ConventionalCommit spec, and actively deviates from the ConventionalChangelog spec, however I would appreciate feedback if possible.

I like convco very much. However, to be of real value to me I need to be able increment either the major, minor or patch via custom types, not constrained to feat and fix. This is allowed by the spec, but not mandated. This PR removes conventional::Type preferring instead for every type to be a String. It then adds a new Increment config Type field which allows the specification of which value to increment when that type is encountered in the git history.

@hdevalke
Copy link
Collaborator

Thank you for the PR. Convco already supports custom types https://github.com/convco/convco/blob/master/src/conventional/commits.rs#L20, but not an increment behavior. I will have a look to your PR later in more details

Copy link
Collaborator

@hdevalke hdevalke left a comment

Choose a reason for hiding this comment

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

don't forget to use a conventional commit message for your commits ;)

src/conventional/config.rs Show resolved Hide resolved
section: "Fixes".into(),
hidden: false,
},
Type {
r#type: "build".into(),
increment: Increment::Patch,
Copy link
Collaborator

Choose a reason for hiding this comment

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

These none fix/feature types should default to the Increment::None. Otherwise this is a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you are saying and will update. Theoretically though, should any new release, even if the interim commits are chore or build or something else minor, not expect at least a patch increment?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Additional types are not mandated by the Conventional Commits specification, and have no implicit effect in Semantic Versioning (unless they include a BREAKING CHANGE).

Unless they contain a breaking change there should not be an implicit effect. If it is configurable it becomes an explicit effect. For example a style: reformatted with new rules should not require a new release imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see that you are following the spec. But I feel there is a big difference between "should not" and "can not". What if this was a continuous deployment scenario and every merge to main gets released and hence a version number? Regardless, this PR has this covered.

src/cmd/version.rs Outdated Show resolved Hide resolved
@ccopsey
Copy link
Contributor Author

ccopsey commented Dec 27, 2023

don't forget to use a conventional commit message for your commits ;)

Thanks for taking the time to review my proposal. Do you prefer every commit to be conventional? I was expecting a squash merge and hence the title of the PR would take contain the label. Would you prefer me to reword my commit via rebase? And hence break the history of the PR?

@hdevalke
Copy link
Collaborator

don't forget to use a conventional commit message for your commits ;)

Thanks for taking the time to review my proposal. Do you prefer every commit to be conventional? I was expecting a squash merge and hence the title of the PR would take contain the label. Would you prefer me to reword my commit via rebase? And hence break the history of the PR?

don't forget to use a conventional commit message for your commits ;)

Thanks for taking the time to review my proposal. Do you prefer every commit to be conventional? I was expecting a squash merge and hence the title of the PR would take contain the label. Would you prefer me to reword my commit via rebase? And hence break the history of the PR?

Indeed I can use the squash and merge. I normally use the rebase and merge as the workflows will make sure a conventional commit is used for each commit.

… a configurable conventional::config::Increment to each conventional::config::Type
@ccopsey
Copy link
Contributor Author

ccopsey commented Jan 1, 2024

Indeed I can use the squash and merge. I normally use the rebase and merge as the workflows will make sure a conventional commit is used for each commit.

Since the workflows failed I fixed the style issues and squashed the lot down to a single conventional commit.

@hdevalke
Copy link
Collaborator

hdevalke commented Jan 7, 2024

I fixed the remaining issues here: https://github.com/convco/convco/tree/BRANCH_NAME

hdevalke pushed a commit that referenced this pull request Jan 7, 2024
This makes it possible to increment the version for different types then
feat and fix.
Also replaces conventional type with `String` type instead of enum.

Refs: #164
hdevalke pushed a commit that referenced this pull request Jan 7, 2024
This makes it possible to increment the version for different types then
feat and fix.
Also replaces conventional type with `String` type instead of enum.

Refs: #164
@hdevalke
Copy link
Collaborator

hdevalke commented Jan 7, 2024

Merged with 607c9b1

@hdevalke hdevalke closed this Jan 7, 2024
@ccopsey ccopsey deleted the increment branch January 14, 2024 13:14
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.

None yet

2 participants