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.

Sign up for GitHub

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
Open

Changelog tool - Implement robust version incrementing logic #679

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fc7819b
Merge get_or_create_release_mut and push_content into push_description
exzachlyvv Nov 8, 2024
0e3e296
implement robust version incrementing logic
exzachlyvv Nov 8, 2024
ee5f987
minor nits
exzachlyvv Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
248 changes: 213 additions & 35 deletions crates/cli-tools/src/changelog.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,32 +68,37 @@ impl Changelog {
fs::write(self.changelog_path(), self.to_string().as_bytes()).await
}

fn push_description(&mut self, severity: Severity, content: &str) -> Result<()> {
let content = if content.starts_with("- ") { content } else { &format!("- {content}") };
let current_release = self.get_or_create_release_mut();
current_release.push_content(severity, content)
fn get_current_release(&self) -> &Release {
exzachlyvv marked this conversation as resolved.
Show resolved Hide resolved
self.releases.first().unwrap()
}

fn get_current_release_mut(&mut self) -> &mut Release {
self.releases.first_mut().unwrap()
}

// Gets newest (first) release. Creates a new one if newest release is not prerelease.
fn get_or_create_release_mut(&mut self) -> &mut Release {
let current_release = self.releases.first().unwrap();
fn push_description(&mut self, severity: Severity, content: &str) -> Result<()> {
let content = if content.starts_with("- ") { content } else { &format!("- {content}") };

// Current version is released, insert a new one.
if current_release.version.pre.is_empty() {
let mut next_version = Version::new(
current_release.version.major,
current_release.version.minor,
current_release.version.patch + 1,
);
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().


next_version.pre = Prerelease::new("git").unwrap();
if is_new_release_needed {
let mut new_release = Release::from(current_release.version.clone());

let new_release = Release::from(next_version);
new_release.increment_version(severity);

self.releases.insert(0, new_release);
}

self.releases.first_mut().unwrap()
// Push content onto release
self.get_current_release_mut()
.contents
.entry(severity)
.or_default()
.push(content.to_string());

Ok(())
}

/// Parses and validates a changelog.
Expand Down Expand Up @@ -181,14 +186,14 @@ impl Changelog {
let path = &self.crate_path;
let metadata = metadata(path).await?;
ensure!(
self.releases.first().unwrap().version == metadata.packages[0].version,
self.get_current_release().version == metadata.packages[0].version,
"Version mismatch between Cargo.toml and CHANGELOG.md for {path}"
);
Ok(())
}

async fn sync_cargo_toml(&self) -> Result<()> {
let expected_version = &self.releases.first().unwrap().version;
let expected_version = &self.get_current_release().version;

let mut sed = Command::new("sed");
sed.arg("-i");
Expand Down Expand Up @@ -266,19 +271,56 @@ struct Release {
contents: BTreeMap<Severity, Vec<String>>,
}

impl Release {
fn push_content(&mut self, severity: Severity, content: &str) -> Result<()> {
self.contents.entry(severity).or_default().push(content.to_string());
Ok(())
}
}

impl From<Version> for Release {
fn from(version: Version) -> Self {
Release { version, contents: BTreeMap::new() }
}
}

impl Release {
fn get_max_severity(&self) -> Severity {
for severity in [Severity::Major, Severity::Minor, Severity::Patch] {
if self.contents.contains_key(&severity) {
return severity;
}
}

Severity::Patch
exzachlyvv marked this conversation as resolved.
Show resolved Hide resolved
}

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.

let mut next_version = self.version.clone();

next_version.pre = Prerelease::new("git").unwrap();

match severity {
Severity::Patch => {
next_version.patch += 1;
}
Severity::Minor => {
if next_version.major == 0 {
next_version.patch += 1;
} else {
next_version.minor += 1;
next_version.patch = 0;
}
}
Severity::Major => {
if next_version.major == 0 {
next_version.minor += 1;
next_version.patch = 0;
} else {
next_version.major += 1;
next_version.minor = 0;
next_version.patch = 0;
}
}
}

self.version = next_version;
}
}

impl Display for Release {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
writeln!(f, "## {}\n", self.version)?;
Expand Down Expand Up @@ -822,7 +864,7 @@ mod tests {
changelog.push_description(Severity::Major, "- testing with dash").unwrap();

assert_eq!(
changelog.get_or_create_release_mut().contents.get(&Severity::Major).unwrap(),
changelog.get_current_release().contents.get(&Severity::Major).unwrap(),
&vec![
"- A change".to_string(),
"- testing no dash".to_string(),
Expand All @@ -832,7 +874,7 @@ mod tests {
}

#[test]
fn get_or_create_release_mut_uses_first_when_prerelease() {
fn push_description_uses_first_release_when_prerelease() {
let changelog_str = r"# Changelog

## 0.2.0-git
Expand All @@ -849,13 +891,13 @@ mod tests {
let mut changelog =
Changelog::parse("", changelog_str).expect("Failed to parse changelog.");

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

assert_eq!(current_release.version, Version::parse("0.2.0-git").unwrap());
assert_eq!(changelog.get_current_release().version, Version::parse("0.2.0-git").unwrap());
}

#[test]
fn get_or_create_release_mut_creates_new_when_current_is_released() {
fn push_description_creates_new_release_when_current_is_released() {
let changelog_str = r"# Changelog

## 0.2.0
Expand All @@ -872,10 +914,146 @@ mod tests {
let mut changelog =
Changelog::parse("", changelog_str).expect("Failed to parse changelog.");

let current_release = changelog.get_or_create_release_mut();
changelog.push_description(Severity::Patch, "asdf").expect("Failed to push description.");

assert_eq!(changelog.get_current_release().version, Version::parse("0.2.1-git").unwrap());
}

#[test]
fn push_description_creates_new_release_when_pushed_severity_is_first_of_higher() {
let changelog_str = r"# Changelog

## 0.1.1-git

### Minor

- A change

## 0.1.0

<!-- Increment to skip CHANGELOG.md test: 0 -->
";

let mut changelog =
Changelog::parse("", changelog_str).expect("Failed to parse changelog.");

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.

}

#[test]
fn push_description_major_increments_stable() {
let changelog_str = r"# Changelog

## 1.0.0

### Major

- A change

## 0.1.0

<!-- Increment to skip CHANGELOG.md test: 0 -->
";

let mut changelog =
Changelog::parse("", changelog_str).expect("Failed to parse changelog.");

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

assert_eq!(changelog.get_current_release().version, Version::parse("2.0.0-git").unwrap());
}

#[test]
fn push_description_major_increments_unstable() {
let changelog_str = r"# Changelog

## 0.1.1

### Minor

- A change

## 0.1.0

<!-- Increment to skip CHANGELOG.md test: 0 -->
";

let mut changelog =
Changelog::parse("", changelog_str).expect("Failed to parse changelog.");

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());
}

#[test]
fn push_description_minor_increments_stable() {
let changelog_str = r"# Changelog

## 1.0.0

### Major

- A change

## 0.1.0

<!-- Increment to skip CHANGELOG.md test: 0 -->
";

let mut changelog =
Changelog::parse("", changelog_str).expect("Failed to parse changelog.");

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

assert_eq!(changelog.get_current_release().version, Version::parse("1.1.0-git").unwrap());
}

#[test]
fn push_description_minor_increments_unstable() {
let changelog_str = r"# Changelog

## 0.1.1

### Minor

- A change

## 0.1.0

<!-- Increment to skip CHANGELOG.md test: 0 -->
";

let mut changelog =
Changelog::parse("", changelog_str).expect("Failed to parse changelog.");

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

assert_eq!(changelog.get_current_release().version, Version::parse("0.1.2-git").unwrap());
}

#[test]
fn push_description_patch_increments() {
let changelog_str = r"# Changelog

## 0.1.1

### Minor

- A change

## 0.1.0

<!-- Increment to skip CHANGELOG.md test: 0 -->
";

let mut changelog =
Changelog::parse("", changelog_str).expect("Failed to parse changelog.");

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

assert_eq!(current_release.version, Version::parse("0.2.1-git").unwrap());
// Assert the new release already inserted
assert_eq!(changelog.releases.len(), 3);
assert_eq!(changelog.get_current_release().version, Version::parse("0.1.2-git").unwrap());
}
}
Loading