-
Notifications
You must be signed in to change notification settings - Fork 20
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
Implement Display for Changelog. Hooked up to parsing validation. #595
base: dev/changelog
Are you sure you want to change the base?
Conversation
Next up is implement all the validation for 'ci'.
…ake sure input string == output string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looks good to me modulo comments.
@@ -29,6 +30,16 @@ pub enum ReleaseType { | |||
Patch, | |||
} | |||
|
|||
impl Display for ReleaseType { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
||
ensure!( | ||
output_string == contents, | ||
"Input string does not equal rendered changelog.\n\nIn: {contents}\n\nOut: {contents}" |
There was a problem hiding this comment.
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.
|
||
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"); |
There was a problem hiding this comment.
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.
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.
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
let releases_string = self.releases.iter().map(|release| format!("{release}")).join("\n\n"); | ||
|
||
write!( |
There was a problem hiding this comment.
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} -->")
let description_string = | ||
descriptions.iter().map(|description| format!("- {description}")).join("\n"); | ||
|
||
write!(f, "\n\n### {release_type}\n\n{description_string}")?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
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}")?; | |
} |
By the way, there will be merge conflicts due to #601 when we'll merge into main. We'll have to decide how to do that since GitHub doesn't support reviewing merge commits. |
Diff will clean up after PR #584 is merged