Formatting improvements to for_file and validate (#42)

* better formatting for multiple ownership validation

* better for file formatting

* version 0.2.1

* handling unowned description

Author: perryqh

Cargo.lock

@@ -1,6 +1,6 @@
 [package]
 name = "codeowners"
-version = "0.2.0"
+version = "0.2.1"
 edition = "2021"
 
 [profile.release]

Cargo.toml

@@ -133,7 +133,7 @@ fn cli() -> Result<(), Error> {
                 _ => {
                     println!("Error: file is owned by multiple teams!");
                     for file_owner in file_owners {
-                        println!("\n{}\n", file_owner);
+                        println!("\n{}", file_owner);
                     }
                 }
             }

src/main.rs

@@ -1,4 +1,5 @@
 use file_owner_finder::FileOwnerFinder;
+use itertools::Itertools;
 use mapper::{OwnerMatcher, Source, TeamName};
 use std::{
     error::Error,
@@ -50,15 +51,20 @@ impl TeamOwnership {
 
 impl Display for FileOwner {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        let sources = self
-            .sources
-            .iter()
-            .map(|source| source.to_string())
-            .collect::<Vec<String>>()
-            .join(", ");
+        let sources = if self.sources.is_empty() {
+            "Unowned".to_string()
+        } else {
+            self.sources
+                .iter()
+                .sorted_by_key(|source| source.to_string())
+                .map(|source| source.to_string())
+                .collect::<Vec<_>>()
+                .join("\n- ")
+        };
+
         write!(
             f,
-            "Team: {}\nTeam YML: {}\nDescription: {}",
+            "Team: {}\nTeam YML: {}\nDescription:\n- {}",
             self.team_name, self.team_config_file_path, sources
         )
     }
@@ -123,6 +129,7 @@ impl Ownership {
         let owners = file_owner_finder.find(Path::new(file_path));
         Ok(owners
             .iter()
+            .sorted_by_key(|owner| owner.team_name.to_lowercase())
             .map(|owner| match self.project.get_team(&owner.team_name) {
                 Some(team) => FileOwner {
                     team_name: owner.team_name.clone(),

src/ownership.rs

@@ -173,16 +173,20 @@ impl Error {
     pub fn messages(&self) -> Vec<String> {
         match self {
             Error::FileWithoutOwner { path } => vec![format!("- {}", path.to_string_lossy())],
-            Error::FileWithMultipleOwners { path, owners } => owners
-                .iter()
-                .flat_map(|owner| {
-                    owner
-                        .sources
-                        .iter()
-                        .map(|source| format!("- {} (owner: {}, source: {})", path.to_string_lossy(), owner.team_name, &source))
-                        .collect_vec()
-                })
-                .collect_vec(),
+            Error::FileWithMultipleOwners { path, owners } => {
+                let path_display = path.to_string_lossy();
+                let mut messages = vec![format!("\n{path_display}")];
+
+                owners
+                    .iter()
+                    .sorted_by_key(|owner| owner.team_name.to_lowercase())
+                    .for_each(|owner| {
+                        messages.push(format!(" owner: {}", owner.team_name));
+                        messages.extend(owner.sources.iter().map(|source| format!("  - {source}")));
+                    });
+
+                vec![messages.join("\n")]
+            }
             Error::CodeownershipFileIsStale => vec![],
             Error::InvalidTeam { name, path } => vec![format!("- {} is referencing an invalid team - '{}'", path.to_string_lossy(), name)],
         }

src/ownership/validator.rs

@@ -16,10 +16,18 @@ fn test_validate() -> Result<(), Box<dyn Error>> {
             CODEOWNERS out of date. Run `codeowners generate` to update the CODEOWNERS file
 
             Code ownership should only be defined for each file in one way. The following files have declared ownership in multiple ways
-            - gems/payroll_calculator/calculator.rb (owner: Payments, source: Owner annotation at the top of the file)
-            - gems/payroll_calculator/calculator.rb (owner: Payroll, source: Owner specified in Team YML's `owned_gems`)
-            - ruby/app/services/multi_owned.rb (owner: Payments, source: Owner annotation at the top of the file)
-            - ruby/app/services/multi_owned.rb (owner: Payroll, source: Owner specified in `ruby/app/services/.codeowner`)
+
+            gems/payroll_calculator/calculator.rb
+             owner: Payments
+              - Owner annotation at the top of the file
+             owner: Payroll
+              - Owner specified in Team YML's `owned_gems`
+
+            ruby/app/services/multi_owned.rb
+             owner: Payments
+              - Owner annotation at the top of the file
+             owner: Payroll
+              - Owner specified in `ruby/app/services/.codeowner`
 
             Found invalid team annotations
             - ruby/app/models/blockchain.rb is referencing an invalid team - 'Web3'
@@ -44,7 +52,9 @@ fn test_for_file() -> Result<(), Box<dyn Error>> {
         .stdout(predicate::eq(indoc! {"
             Team: Unowned
             Team YML: Unowned
-            Description: \n"})); // trailing whitespace
+            Description:
+            - Unowned
+            "}));
     Ok(())
 }
 
@@ -57,17 +67,18 @@ fn test_for_file_multiple_owners() -> Result<(), Box<dyn Error>> {
         .arg("ruby/app/services/multi_owned.rb")
         .assert()
         .success()
-        .stdout(predicate::str::starts_with("Error: file is owned by multiple teams!"))
-        // order not static
-        .stdout(predicate::str::contains(indoc! {"
-            Team: Payroll
-            Team YML: config/teams/payroll.yml
-            Description: Owner specified in `ruby/app/services/.codeowner`
-        "}))
-        .stdout(predicate::str::contains(indoc! {"
+        .stdout(predicate::eq(indoc! {"
+            Error: file is owned by multiple teams!
+
             Team: Payments
             Team YML: config/teams/payments.yml
-            Description: Owner annotation at the top of the file
+            Description:
+            - Owner annotation at the top of the file
+
+            Team: Payroll
+            Team YML: config/teams/payroll.yml
+            Description:
+            - Owner specified in `ruby/app/services/.codeowner`
         "}));
     Ok(())
 }

tests/invalid_project_test.rs

@@ -46,11 +46,51 @@ fn test_for_file() -> Result<(), Box<dyn Error>> {
         .stdout(predicate::eq(indoc! {"
             Team: Payroll
             Team YML: config/teams/payroll.yml
-            Description: Owner annotation at the top of the file
+            Description:
+            - Owner annotation at the top of the file
         "}));
     Ok(())
 }
 
+#[test]
+fn test_for_file_same_team_multiple_ownerships() -> Result<(), Box<dyn Error>> {
+    Command::cargo_bin("codeowners")?
+        .arg("--project-root")
+        .arg("tests/fixtures/valid_project")
+        .arg("for-file")
+        .arg("javascript/packages/PayrollFlow/index.tsx")
+        .assert()
+        .success()
+        .stdout(predicate::eq(indoc! {"
+            Team: Payroll
+            Team YML: config/teams/payroll.yml
+            Description:
+            - Owner annotation at the top of the file
+            - Owner defined in `javascript/packages/PayrollFlow/package.json` with implicity owned glob: `javascript/packages/PayrollFlow/**/**`
+        "}));
+    Ok(())
+}
+
+#[test]
+fn test_for_file_with_2_ownerships() -> Result<(), Box<dyn Error>> {
+    Command::cargo_bin("codeowners")?
+        .arg("--project-root")
+        .arg("tests/fixtures/valid_project")
+        .arg("for-file")
+        .arg("javascript/packages/PayrollFlow/index.tsx")
+        .assert()
+        .success()
+        .stdout(predicate::eq(indoc! {"
+            Team: Payroll
+            Team YML: config/teams/payroll.yml
+            Description:
+            - Owner annotation at the top of the file
+            - Owner defined in `javascript/packages/PayrollFlow/package.json` with implicity owned glob: `javascript/packages/PayrollFlow/**/**`
+        "}));
+
+    Ok(())
+}
+
 #[test]
 fn test_for_team() -> Result<(), Box<dyn Error>> {
     Command::cargo_bin("codeowners")?