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

Jump to bottom

Implement Display for Changelog. Hooked up to parsing validation. #595

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

Implement Display for Changelog. Hooked up to parsing validation. #595

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
84517ca
Add "changelog" xtask script to automate updating CHANGELOG.md files.
exzachlyvv Aug 23, 2024
e584ce0
Split changelog xtask into 2 sub commands: 'ci' and 'change'.
exzachlyvv Aug 23, 2024
6922aa6
add more changelog validation.
exzachlyvv Aug 23, 2024
a360f1e
improve changelog parsing logic - line by line, while peeking at next.
exzachlyvv Aug 26, 2024
6c717be
lots of updates based on Julien's feedback.
exzachlyvv Aug 29, 2024
10d2c62
Implement Display for Changelog. Hooked up to parsing validation to m…
exzachlyvv Aug 30, 2024
0d257ae
Merge branch 'dev/changelog' into HEAD
ia0 Sep 2, 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
12 changes: 11 additions & 1 deletion crates/xtask/Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/xtask/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ edition = "2021"
anyhow = "1.0.86"
clap = { version = "4.5.4", features = ["derive"] }
env_logger = "0.11.3"
itertools = "0.13.0"
lazy_static = "1.4.0"
log = "0.4.21"
probe-rs = "0.24.0"
Expand Down
134 changes: 121 additions & 13 deletions crates/xtask/src/changelog.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,12 @@

use core::str;
use std::collections::BTreeMap;
use std::fmt::Write;
use std::fmt::{Display, Write};
use std::io::BufRead;
use std::process::Command;

use anyhow::{anyhow, bail, ensure, Context, Result};
use itertools::Itertools;
use semver::Version;
use wasefire_cli_tools::{cmd, fs};

Expand All @@ -29,6 +30,16 @@ pub enum ReleaseType {
Patch,
}

impl Display for ReleaseType {
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated but I think it's worth doing it soon, is to rename ReleaseType to Scope to be a bit more abstract, because this type is used for 2 things:

  • Describe the scope or impact of a change.
  • Describe the highest scope of a change between 2 releases.

And the second thing is actually only a consequence of the first, so the name should be more relevant to the first, which is describing the scope/impact of a change.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, we could also use the name Severity since it's used here.

fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
Self::Major => write!(f, "Major"),
Self::Minor => write!(f, "Minor"),
Self::Patch => write!(f, "Patch"),
}
}
}

#[derive(Debug, Default, PartialEq, Eq)]
struct Changelog {
releases: Vec<Release>,
Expand Down Expand Up @@ -154,7 +165,28 @@ impl Changelog {
);
ensure!(last_release.contents.is_empty(), "The last release must contain no changes");

Ok(Changelog { releases, skip_counter })
let changelog = Changelog { releases, skip_counter };
let output_string = format!("{changelog}");

ensure!(
output_string == contents,
"Input string does not equal rendered changelog.\n\nIn: {contents}\n\nOut: {contents}"
Copy link
Member

Choose a reason for hiding this comment

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

It's probably more readable to use the similar-asserts crate like here. Use the same version as the wire-derive crate does. Ideally we should have a test for that, but it's not there yet.

);

Ok(changelog)
}
}

impl Display for Changelog {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
let releases_string = self.releases.iter().map(|release| format!("{release}")).join("\n\n");
Copy link
Member

Choose a reason for hiding this comment

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

This is not efficient to build a string for each version and then join them. We want to output directly to the formatter.

Suggested change
let releases_string = self.releases.iter().map(|release| format!("{release}")).join("\n\n");
for release in &self.releases {
writeln!(f, "{release}\n")?;
}

Also this permits getting rid of itertools which is usually not useful.


write!(
Copy link
Member

Choose a reason for hiding this comment

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

Same, we can split this in 2 around the for-loop above.

writeln!(f, "# Changelog\n")?;
for release in &self.releases { ... }
writeln!(f, "<!-- Increment ...: {skip_counter} -->")

f,
"# Changelog\n\n{releases_string}\n\n<!-- Increment to skip CHANGELOG.md test: {} \
-->\n",
self.skip_counter
)
}
}

Expand All @@ -171,6 +203,21 @@ impl Release {
}
}

impl Display for Release {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "## {}", self.version)?;

for (release_type, descriptions) in &self.contents {
let description_string =
descriptions.iter().map(|description| format!("- {description}")).join("\n");

write!(f, "\n\n### {release_type}\n\n{description_string}")?;
Comment on lines +211 to +214
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Suggested change
let description_string =
descriptions.iter().map(|description| format!("- {description}")).join("\n");
write!(f, "\n\n### {release_type}\n\n{description_string}")?;
writeln!(f, "### {scope}\n")?;
for description in descriptions {
writeln!(f, "- {description}")?;
}

}

Ok(())
}
}

/// Validates all changelog files.
pub fn execute_ci() -> Result<()> {
let paths = cmd::output(Command::new("git").args(["ls-files", "*/CHANGELOG.md"]))?;
Expand Down Expand Up @@ -239,7 +286,8 @@ mod tests {

## 0.1.0

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

assert_eq!(
Changelog::parse(changelog).expect("Failed to parse changelog."),
Expand Down Expand Up @@ -289,6 +337,55 @@ mod tests {
);
}

#[test]
fn write_changelog_success() {
let changelog = r"# Changelog

## 0.3.0

### Major

- major update 1
- major update 2

### Minor

- minor update 1
- minor update 2

### Patch

- patch update 1
- patch update 2

## 0.2.0

### Major

- major update 1
- major update 2

### Minor

- minor update 1
- minor update 2

### Patch

- patch update 1
- patch update 2

## 0.1.0

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

assert_eq!(
format!("{}", Changelog::parse(changelog).expect("Failed to parse changelog.")),
changelog
);
}

#[test]
fn parse_changelog_with_missing_release_type_success() {
let changelog = r"# Changelog
Expand Down Expand Up @@ -316,7 +413,8 @@ mod tests {

## 0.1.0

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

assert_eq!(
Changelog::parse(changelog).expect("Failed to parse changelog."),
Expand Down Expand Up @@ -368,7 +466,8 @@ mod tests {

## 0.1.0

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

assert_eq!(
Changelog::parse(changelog).expect("Failed to parse changelog."),
Expand Down Expand Up @@ -407,7 +506,8 @@ mod tests {

## 0.1.0

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

assert!(Changelog::parse(changelog)
.expect_err("Parse changelog was successful.")
Expand All @@ -421,7 +521,8 @@ mod tests {

## 0.1.0

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

assert_eq!(
Changelog::parse(changelog).expect("Failed to parse changelog."),
Expand All @@ -438,9 +539,11 @@ mod tests {
#[test]
fn parse_changelog_handles_skip_counter_at_end() {
let changelog = r"# Changelog

## 0.1.0

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

assert_eq!(
Changelog::parse(changelog).expect("Failed to parse changelog."),
Expand Down Expand Up @@ -492,7 +595,8 @@ mod tests {

- A change

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

assert!(Changelog::parse(changelog)
.expect_err("Parse changelog was successful.")
Expand All @@ -504,7 +608,8 @@ mod tests {
fn parse_changelog_versions_requires_at_least_one_release() {
let changelog = r"# Changelog

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

assert!(Changelog::parse(changelog)
.expect_err("Parse changelog was successful.")
Expand All @@ -522,7 +627,8 @@ mod tests {

- A change

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

assert!(Changelog::parse(changelog)
.expect_err("Parse changelog was successful.")
Expand All @@ -540,7 +646,8 @@ mod tests {

- A change

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

assert!(Changelog::parse(changelog)
.expect_err("Parse changelog was successful.")
Expand All @@ -564,7 +671,8 @@ mod tests {

- A change

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

assert!(Changelog::parse(changelog)
.expect_err("Parse changelog was successful.")
Expand Down
Loading