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

Changelog tool - Implement robust version incrementing logic #679

Open
wants to merge 3 commits into
base: dev/changelog
Choose a base branch
from

Conversation

exzachlyvv
Copy link
Collaborator

No description provided.

@exzachlyvv exzachlyvv requested a review from ia0 as a code owner November 8, 2024 17:13
);
let current_release = self.get_current_release();
let is_new_release_needed =
current_release.version.pre.is_empty() || current_release.get_max_severity() > severity;
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't sound right to me, I would expect only the pre.is_empty() check to be needed. In the other case, we don't want a new release but just to call increment_version().

crates/cli-tools/src/changelog.rs Show resolved Hide resolved
crates/cli-tools/src/changelog.rs Show resolved Hide resolved
Severity::Patch
}

fn increment_version(&mut self, severity: Severity) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this function will be useful once the test above is fixed. We probably want 2 functions:

  • One that creates a pre-release from a release (just incrementing patch and adding -git).
  • One that makes sure a pre-release is at least a given severity.

That last function could really just be:

  • If version.major == 0 then reduce severity by one (major to minor, minor to patch, patch to patch).
  • If severity is patch then do nothing and exit.
  • If version.patch > 0 then increment version.minor and reset version.patch.
  • If severity is minor then do nothing and exit.
  • If version.minor > 0 then increment version.major and reset version.minor.
  • Do nothing and exit (we know severity is major).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm struggling to understand at a higher level. Could you explain how you see those 2 functions being used?

I think some examples of when we want to create a new release vs just increment version would be helpful.

Copy link
Member

Choose a reason for hiding this comment

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

In push_description() you have:

        let is_new_release_needed =
            current_release.version.pre.is_empty() || current_release.get_max_severity() > severity;

Those 2 conditions should not be conflated because they result in different actions:

  • If the latest version is not a pre-release, then one should be created (patch bump).
  • If the severity is "higher" (i.e. smaller) than the "higest" (i.e. lowest) severity of the pre-release, then only the version needs to be updated (and appropriate severity section added).

More concretely, using the format <latest version> <change severity> -> <action>:

  • 1.0.0 patch -> create 1.0.1-git with Patch
  • 1.0.0 minor -> create 1.1.0-git with Minor
  • 1.0.0 major -> create 2.0.0-git with Major
  • 1.2.3-git patch -> nothing
  • 1.2.3-git minor -> update from 1.2.3-git to 1.3.0-git inserting Minor
  • 1.2.3-git major -> update from 1.2.3-git to 2.0.0-git inserting Major
  • 0.1.0 minor -> create 0.1.1-git with Minor (same for patch/Patch)
  • 0.1.0 major -> create 0.2.0-git with Major
  • 0.1.2-git minor -> insert Minor if absent (same for patch/Patch)
  • 0.1.2-git major -> update from 0.1.2-git to 0.2.0-git inserting Major

An action of create means to prepend a new version. An action of update means to simple change the version without changing the list of versions.


changelog.push_description(Severity::Major, "asdf").expect("Failed to push description.");

assert_eq!(changelog.get_current_release().version, Version::parse("0.2.0-git").unwrap());
Copy link
Member

Choose a reason for hiding this comment

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

We broke the invariant that we have only one pre-release per changelog. It's not enough to check the latest release.

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.

2 participants