imp(all): Minor codebase improvements (#53)

* address linters * add format to makefile * more adjustments * more improvements and removing todos * more improvements * another improvement * remove unnecessary todo * add changelog entry
MalteHerrmann · Jul 25, 2024 · c985eab · c985eab
1 parent 136c8cd
commit c985eab
Show file tree

Hide file tree

Showing 10 changed files with 75 additions and 111 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,6 +12,7 @@ This changelog was created using the `clu` binary
 
 ### Improvements
 
+- (all) [#53](https://github.com/MalteHerrmann/changelog-utils/pull/53) Minor codebase improvements.
 - (crud) [#48](https://github.com/MalteHerrmann/changelog-utils/pull/48) Use authenticated requests when checking open PRs.
 - (config) [#51](https://github.com/MalteHerrmann/changelog-utils/pull/51) Get available configuration from existing changelog during initialization.
 

diff --git a/Makefile b/Makefile
@@ -6,6 +6,10 @@ test:
 build:
 	cargo build --locked
 
+# Format codebase
+format:
+	cargo fmt
+
 # Install binary
 install:
 	cargo install --path . --locked

diff --git a/src/change_type.rs b/src/change_type.rs
@@ -34,28 +34,28 @@ pub fn parse(config: config::Config, line: &str) -> Result<ChangeType, ChangeTyp
     let mut fixed_name = name.to_string();
     let mut problems: Vec<String> = Vec::new();
 
-    let mut found = false;
-    // TODO: this should probably be done with map or smth. more rusty
-    for (change_type, pattern) in config.change_types.iter() {
-        if RegexBuilder::new(pattern)
+    // Check if the correctness of the current change type.
+    if !config.change_types.iter().any(|(change_type, pattern)| {
+        if !RegexBuilder::new(pattern)
             .case_insensitive(true)
-            .build()?
+            .build()
+            .unwrap()
             .is_match(name)
         {
-            if name != change_type {
-                problems.push(format!(
-                    "'{change_type}' should be used instead of '{name}'"
-                ));
-                change_type.clone_into(&mut fixed_name);
-            }
-            found = true;
-            break;
+            return false;
+        }
+
+        if name != change_type {
+            problems.push(format!(
+                "'{change_type}' should be used instead of '{name}'"
+            ));
+            change_type.clone_into(&mut fixed_name);
         }
-    }
 
-    if !found {
+        true
+    }) {
         problems.push(format!("'{name}' is not a valid change type"))
-    }
+    };
 
     let fixed = format!("### {fixed_name}");
     if format!("### {name}").ne(line) {

diff --git a/src/changelog.rs b/src/changelog.rs
@@ -10,7 +10,6 @@ use std::{
 #[derive(Debug)]
 pub struct Changelog {
     pub path: PathBuf,
-    pub fixed: Vec<String>,
     comments: Vec<String>,
     legacy_contents: Vec<String>,
     pub releases: Vec<release::Release>,
@@ -20,11 +19,11 @@ pub struct Changelog {
 impl Changelog {
     /// Exports the changelog contents to the given filepath.
     pub fn write(&self, export_path: &Path) -> Result<(), ChangelogError> {
-        Ok(fs::write(export_path, self.get_fixed())?)
+        Ok(fs::write(export_path, self.get_fixed_contents())?)
     }
 
     /// Returns the fixed contents as a String to be exported.
-    pub fn get_fixed(&self) -> String {
+    pub fn get_fixed_contents(&self) -> String {
         let mut exported_string = "".to_string();
 
         self.comments
@@ -81,7 +80,6 @@ pub fn parse_changelog(config: Config, file_path: &Path) -> Result<Changelog, Ch
     let mut n_change_types = 0;
 
     let mut comments: Vec<String> = Vec::new();
-    let mut fixed: Vec<String> = Vec::new();
     let mut legacy_contents: Vec<String> = Vec::new();
     let mut releases: Vec<release::Release> = Vec::new();
     let mut problems: Vec<String> = Vec::new();
@@ -114,7 +112,6 @@ pub fn parse_changelog(config: Config, file_path: &Path) -> Result<Changelog, Ch
         if is_comment && exit_comment_regex.is_match(trimmed_line) {
             is_comment = false;
             comments.push(line.to_string());
-            fixed.push(line.to_string());
 
             // Check inline comments
             if let Some(e) = escapes::check_escape_pattern(trimmed_line) {
@@ -125,7 +122,6 @@ pub fn parse_changelog(config: Config, file_path: &Path) -> Result<Changelog, Ch
         }
 
         if is_comment {
-            fixed.push(line.to_string());
             comments.push(line.to_string());
             continue;
         }
@@ -135,18 +131,19 @@ pub fn parse_changelog(config: Config, file_path: &Path) -> Result<Changelog, Ch
 
             releases.push(current_release.clone());
             n_releases += 1;
-            match seen_releases.contains(&current_release.version) {
-                true => add_to_problems(
+            if seen_releases.contains(&current_release.version) {
+                add_to_problems(
                     &mut problems,
                     file_path,
                     i,
                     format!("duplicate release: {}", &current_release.version),
-                ),
-                false => seen_releases.push((current_release.version).to_string()),
+                );
+            } else {
+                seen_releases.push((current_release.version).to_string());
             };
 
             // reset the seen change types for the current release
-            seen_change_types = Vec::new();
+            seen_change_types.clear();
             n_change_types = 0;
 
             if current_release
@@ -161,16 +158,12 @@ pub fn parse_changelog(config: Config, file_path: &Path) -> Result<Changelog, Ch
                 .into_iter()
                 .for_each(|p| add_to_problems(&mut problems, file_path, i, p.to_string()));
 
-            fixed.push(current_release.fixed);
-
             continue;
         }
 
         if trimmed_line.starts_with("### ") {
             current_change_type = change_type::parse(config.clone(), line)?;
 
-            // TODO: this handling should definitely be improved.
-            // It's only a quick and dirty implementation for now.
             n_change_types += 1;
             if seen_change_types.contains(&current_change_type.name) {
                 add_to_problems(
@@ -187,26 +180,21 @@ pub fn parse_changelog(config: Config, file_path: &Path) -> Result<Changelog, Ch
                 seen_change_types.push(current_change_type.name.clone());
             }
 
-            fixed.push(current_change_type.fixed.clone());
-
             current_change_type
                 .problems
                 .iter()
                 .for_each(|p| add_to_problems(&mut problems, file_path, i, p.to_string()));
 
-            // TODO: improve this? can this handling be made "more rustic"?
             let last_release = releases
                 .get_mut(n_releases - 1)
                 .expect("failed to get last release");
+
             last_release.change_types.push(current_change_type.clone());
 
             continue;
         }
 
-        // TODO: check how to handle legacy content with the type based export?
-        // TODO: this can actually be removed now with the new type-based exports
-        if !trimmed_line.starts_with('-') || is_legacy {
-            fixed.push(line.to_string());
+        if !trimmed_line.starts_with('-') {
             continue;
         }
 
@@ -216,7 +204,6 @@ pub fn parse_changelog(config: Config, file_path: &Path) -> Result<Changelog, Ch
                 if !escapes.contains(&escapes::LinterEscape::FullLine) {
                     add_to_problems(&mut problems, file_path, i, err.to_string());
                 }
-                fixed.push(line.to_string());
 
                 // reset escapes after processing entry
                 escapes.clear();
@@ -225,7 +212,6 @@ pub fn parse_changelog(config: Config, file_path: &Path) -> Result<Changelog, Ch
             }
         };
 
-        // TODO: ditto, handling could be improved here like with change types, etc.
         if seen_prs.contains(&current_entry.pr_number)
             && (!escapes.contains(&escapes::LinterEscape::DuplicatePR)
                 && !escapes.contains(&escapes::LinterEscape::FullLine))
@@ -248,10 +234,6 @@ pub fn parse_changelog(config: Config, file_path: &Path) -> Result<Changelog, Ch
                 .for_each(|p| add_to_problems(&mut problems, file_path, i, p.to_string()));
         }
 
-        // TODO: can be removed with new type-based exports
-        fixed.push(current_entry.clone().fixed);
-
-        // TODO: improve this, seems not ideal because it's also being retrieved in the statements above
         let last_release = releases
             .get_mut(n_releases - 1)
             .expect("failed to get last release");
@@ -260,6 +242,7 @@ pub fn parse_changelog(config: Config, file_path: &Path) -> Result<Changelog, Ch
             .change_types
             .get_mut(n_change_types - 1)
             .expect("failed to get last change type");
+
         last_change_type.entries.push(current_entry);
 
         // Reset the escapes after an entry line
@@ -268,7 +251,6 @@ pub fn parse_changelog(config: Config, file_path: &Path) -> Result<Changelog, Ch
 
     Ok(Changelog {
         path: file_path.to_path_buf(),
-        fixed,
         releases,
         comments,
         problems,
@@ -311,7 +293,6 @@ mod changelog_tests {
 
         let mut cl = Changelog {
             path: PathBuf::from_str("test").unwrap(),
-            fixed: Vec::new(),
             releases: Vec::new(),
             comments: Vec::new(),
             legacy_contents: Vec::new(),
@@ -369,25 +350,19 @@ pub fn get_settings_from_existing_changelog(config: &mut Config, contents: &str)
         let trimmed_line = line.trim();
 
         if trimmed_line.starts_with("### ") {
-            match change_type::parse(config.clone(), line) {
-                Ok(ct) => {
-                    if !seen_change_types.contains(&ct.name) {
-                        seen_change_types.push(ct.name)
-                    }
+            if let Ok(ct) = change_type::parse(config.clone(), line) {
+                if !seen_change_types.contains(&ct.name) {
+                    seen_change_types.push(ct.name)
                 }
-                _ => (),
             };
 
             continue;
         }
 
-        match entry::parse(config, line) {
-            Ok(e) => {
-                if !seen_categories.contains(&e.category) {
-                    seen_categories.push(e.category)
-                }
+        if let Ok(e) = entry::parse(config, line) {
+            if !seen_categories.contains(&e.category) {
+                seen_categories.push(e.category)
             }
-            _ => (),
         }
     }
 

diff --git a/src/config.rs b/src/config.rs
@@ -5,8 +5,6 @@ use std::{collections::BTreeMap, fmt, fs, path::Path};
 use url::Url;
 
 /// Holds the configuration of the application
-///
-/// TODO: check if clone is actually necessary?
 #[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct Config {
     /// The list of categories for a given entry,
@@ -203,7 +201,6 @@ mod config_adjustment_tests {
     fn test_add_category_duplicate() {
         let mut config = load_example_config();
         assert_eq!(config.categories.len(), 2);
-        // TODO: get value from existing categories instead of hardcoding here
         assert_eq!(
             add_category(&mut config, "test".to_string()).unwrap_err(),
             ConfigAdjustError::CategoryAlreadyFound
@@ -215,7 +212,6 @@ mod config_adjustment_tests {
     fn test_remove_category() {
         let mut config = load_example_config();
         assert_eq!(config.categories.len(), 2);
-        // TODO: get value from existing categories instead of hardcoding here
         assert!(remove_category(&mut config, "test".to_string()).is_ok());
         assert_eq!(config.categories.len(), 1);
     }
@@ -224,7 +220,6 @@ mod config_adjustment_tests {
     fn test_remove_category_not_found() {
         let mut config = load_example_config();
         assert_eq!(config.categories.len(), 2);
-        // TODO: get value from existing categories instead of hardcoding here
         assert_eq!(
             remove_category(&mut config, "not-found".to_string()).unwrap_err(),
             ConfigAdjustError::NotFound

diff --git a/src/entry.rs b/src/entry.rs
@@ -14,8 +14,6 @@ pub struct Entry {
     /// The PR number for the given change.
     pub pr_number: u16,
     /// The list of problems with the given line.
-    ///
-    /// TODO: Should this rather be a Vec<a' str>?
     pub problems: Vec<String>,
 }
 
@@ -92,7 +90,7 @@ pub fn parse(config: &config::Config, line: &str) -> Result<Entry, EntryError> {
 
     Ok(Entry {
         category: fixed_category.to_string(),
-        fixed, // TODO: why is it not possible to have this as &'a str too?
+        fixed,
         pr_number,
         problems,
     })
@@ -187,7 +185,12 @@ fn check_spelling(config: &config::Config, text: &str) -> (String, Vec<String>)
                 };
 
                 fixed = compile_regex(pattern)
-                    .expect("failed to compile regex") // TODO: return Result rather than use expect here?
+                    .unwrap_or_else(|_| {
+                        panic!(
+                            "failed to compile regex for '{}'; check spelling configuration",
+                            pattern
+                        )
+                    })
                     .replace(fixed.as_str(), correct_spelling)
                     .to_string();
 
@@ -326,7 +329,6 @@ mod entry_tests {
     #[test]
     fn test_malformed_entry() {
         let example = r"- (cli) [#13tps://github.com/Ma/2";
-        // TODO: figure how to still return an entry but with the corresponding array of problems filled
         assert!(parse(&load_test_config(), example).is_err());
     }
 
@@ -496,9 +498,8 @@ mod spelling_tests {
         let (fixed, problems) = check_spelling(&load_test_config(), example);
         assert_eq!(fixed, "Fix API and CLI.");
         assert_eq!(problems.len(), 2);
-        // TODO: this is currently not deterministically in the same order
-        assert!(problems.contains(&"'API' should be used instead of 'aPi'".to_string()));
-        assert!(problems.contains(&"'CLI' should be used instead of 'ClI'".to_string()));
+        assert_eq!(problems[0], "'API' should be used instead of 'aPi'");
+        assert_eq!(problems[1], "'CLI' should be used instead of 'ClI'");
     }
 
     #[test]

diff --git a/src/escapes.rs b/src/escapes.rs
@@ -9,20 +9,21 @@ pub enum LinterEscape {
 
 /// Checks the given comment for an escape pattern.
 pub fn check_escape_pattern(line: &str) -> Option<LinterEscape> {
-    // TODO: improve handling here with associated traits for the linter escapes?
-    match Regex::new(r"<!--\s*clu-disable-next-line-duplicate-pr(:.+)?\s*-->")
+    if Regex::new(r"<!--\s*clu-disable-next-line-duplicate-pr(:.+)?\s*-->")
         .unwrap()
         .is_match(line)
     {
-        true => Some(LinterEscape::DuplicatePR),
-        false => match Regex::new(r"<!--\s*clu-disable-next-line(:.+)?\s*-->")
-            .unwrap()
-            .is_match(line)
-        {
-            true => Some(LinterEscape::FullLine),
-            false => None,
-        },
+        return Some(LinterEscape::DuplicatePR);
     }
+
+    if Regex::new(r"<!--\s*clu-disable-next-line(:.+)?\s*-->")
+        .unwrap()
+        .is_match(line)
+    {
+        return Some(LinterEscape::FullLine);
+    }
+
+    None
 }
 
 #[cfg(test)]