Merge pull request #264 from helsing-ai/rene/issue-176-lints

issue-176: add lint to keep service defs out of lib type packages

src/validation/rules.rs

@@ -14,15 +14,19 @@
 
 use std::fmt::Debug;
 
+use lib_package::LibPackage;
+
 use crate::{
     manifest::PackageManifest,
+    package::PackageType,
     validation::{
         data::*,
         violation::{self, *},
     },
 };
 
 mod ident_casing;
+mod lib_package;
 mod package_name;
 
 pub use self::{ident_casing::*, package_name::*};
@@ -104,8 +108,57 @@ impl Rule for RuleSet {
 
 /// Get default rules for a given `buffrs` package name.
 pub fn all(manifest: &PackageManifest) -> RuleSet {
-    vec![
+    let mut ret: Vec<Box<dyn Rule>> = vec![
         Box::new(PackageName::new(manifest.name.clone())),
         Box::new(IdentCasing),
-    ]
+    ];
+
+    if manifest.kind == PackageType::Lib {
+        ret.push(Box::new(LibPackage));
+    }
+
+    ret
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::package::PackageType;
+    use semver::Version;
+
+    #[test]
+    fn all_should_not_contain_libpackage_rule_for_api_type_packages(
+    ) -> Result<(), Box<dyn std::error::Error>> {
+        let manifest = PackageManifest {
+            kind: PackageType::Api,
+            name: crate::package::PackageName::new("package")?,
+            version: Version::new(0, 1, 0),
+            description: Default::default(),
+        };
+
+        let all = all(&manifest);
+        assert!(all
+            .iter()
+            .all(|rule| rule.rule_name() != LibPackage.rule_name()));
+
+        Ok(())
+    }
+
+    #[test]
+    fn all_should_contain_libpackage_rule_for_lib_type_packages(
+    ) -> Result<(), Box<dyn std::error::Error>> {
+        let manifest = PackageManifest {
+            kind: PackageType::Lib,
+            name: crate::package::PackageName::new("package")?,
+            version: Version::new(0, 1, 0),
+            description: Default::default(),
+        };
+
+        let all = all(&manifest);
+        assert!(all
+            .iter()
+            .any(|rule| rule.rule_name() == LibPackage.rule_name()));
+
+        Ok(())
+    }
 }

src/validation/rules/lib_package.rs

@@ -0,0 +1,80 @@
+use super::*;
+
+#[derive(Debug, Clone, Copy)]
+pub struct LibPackage;
+
+impl Rule for LibPackage {
+    fn rule_info(&self) -> &'static str {
+        "Make sure that lib packages don't contain service definitions."
+    }
+
+    fn rule_level(&self) -> Level {
+        Level::Error
+    }
+
+    fn check_package(&mut self, package: &Package) -> Violations {
+        let package_name = &package.name;
+        package
+            .entities
+            .iter()
+            .filter_map(|(name, entity)| {
+                if let Entity::Service(_) = entity {
+                    let message = violation::Message {
+                        message: format!("{name} is a service definition but {package_name} is a lib type package and thus shouldn't contain services."),
+                        help: "It's best to keep packages containing services separate from packages containing messages and enums.".into(),
+                    };
+                    Some(self.to_violation(message))
+                } else {
+                    None
+                }
+            })
+            .collect()
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use std::collections::BTreeMap;
+
+    use super::*;
+
+    #[test]
+    fn should_complain_about_service_defs() {
+        let entities = {
+            let mut ret: BTreeMap<String, Entity> = BTreeMap::new();
+            ret.insert("service A".into(), Entity::Service(Service {}));
+            ret.insert("service B".into(), Entity::Service(Service {}));
+
+            ret
+        };
+
+        let package = Package {
+            name: "my package".into(),
+            files: vec!["ignored.proto".into()],
+            entities,
+        };
+
+        let violations = LibPackage.check_package(&package);
+
+        let help = "It's best to keep packages containing services separate from packages containing messages and enums.";
+        let violation_1 = Violation {
+            rule: "LibPackage".into(),
+            level: Level::Error,
+            message: violation::Message {
+                message: "service A is a service definition but my package is a lib type package and thus shouldn't contain services.".into(),
+                help: help.into(),
+            },
+            location: Default::default(),
+            info: LibPackage.rule_info().into(),
+        };
+        let violation_2 = Violation {
+            message: violation::Message {
+                message: "service B is a service definition but my package is a lib type package and thus shouldn't contain services.".into(),
+                help: help.into(),
+            },
+            ..violation_1.clone()
+        };
+
+        assert_eq!(violations, vec![violation_1, violation_2]);
+    }
+}