From c7a16dc82f739ccb8c5cbc2b87cdfdda4e90063a Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sun, 15 Nov 2026 02:14:26 +0000 Subject: [PATCH 1/8] wip: SQBRPYTHON provider layout rules (unbuilt draft) --- .../providers/carrier_api/provider.py | 16 ++ .../providers/carrier_api/provider.py | 16 ++ .../violations/providers/stock_feed.py | 1 + .../src/node_units/_helpers/functions.rs | 2 +- .../src/node_units/main/node_unit_module.rs | 25 +- .../src/node_units/models.rs | 14 +- .../engine/tests/test_incremental_rules.rs | 3 + .../src/python_rules/_helpers/units/mod.rs | 1 + .../python_rules/_helpers/units/modules.rs | 6 +- .../python_rules/_helpers/units/project.rs | 18 +- .../python_rules/_helpers/units/providers.rs | 226 ++++++++++++++++++ .../src/python_rules/constants.rs | 65 ++++- .../main/evaluate_python_rules.rs | 6 + .../sqlbuild-rules/src/python_rules/tests.rs | 2 + .../python_rules/tests/test_provider_rules.rs | 149 ++++++++++++ .../tests/test_python_catalogue.rs | 6 +- .../docs/concepts/python-nodes/providers.mdx | 4 + .../docs/docs/concepts/rules/python-rules.mdx | 33 +++ 18 files changed, 577 insertions(+), 16 deletions(-) create mode 100644 benchmarks/duckshop/edge_cases/python_rules/conforming/providers/carrier_api/provider.py create mode 100644 benchmarks/duckshop/edge_cases/python_rules/violations/providers/carrier_api/provider.py create mode 100644 benchmarks/duckshop/edge_cases/python_rules/violations/providers/stock_feed.py create mode 100644 crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs create mode 100644 crates/sqlbuild-rules/src/python_rules/tests/test_provider_rules.rs diff --git a/benchmarks/duckshop/edge_cases/python_rules/conforming/providers/carrier_api/provider.py b/benchmarks/duckshop/edge_cases/python_rules/conforming/providers/carrier_api/provider.py new file mode 100644 index 000000000..f05a049e5 --- /dev/null +++ b/benchmarks/duckshop/edge_cases/python_rules/conforming/providers/carrier_api/provider.py @@ -0,0 +1,16 @@ +"""Carrier pickup client.""" + +from python.utils.currency import amounts +from sqlbuild.providers import Provider + +_MINIMUM_CHARGE_CENTS: int = 250 + + +class CarrierApi(Provider): + """Client for the carrier pickup API.""" + + endpoint: str = "https://carrier.example.com" + + def minimum_charge(self) -> str: + """Return the minimum pickup charge as text.""" + return amounts.cents_text(_MINIMUM_CHARGE_CENTS) diff --git a/benchmarks/duckshop/edge_cases/python_rules/violations/providers/carrier_api/provider.py b/benchmarks/duckshop/edge_cases/python_rules/violations/providers/carrier_api/provider.py new file mode 100644 index 000000000..290a2b7fd --- /dev/null +++ b/benchmarks/duckshop/edge_cases/python_rules/violations/providers/carrier_api/provider.py @@ -0,0 +1,16 @@ +"""Carrier pickup client.""" + +from python.inventory.restock.main import restock +from sqlbuild.providers import Provider + + +def pickup_label(sku: str) -> str: + """Return the pickup label of one product.""" + restock(sku) + return sku + + +class CarrierApi(Provider): + """Client for the carrier pickup API.""" + + endpoint: str = "https://carrier.example.com" diff --git a/benchmarks/duckshop/edge_cases/python_rules/violations/providers/stock_feed.py b/benchmarks/duckshop/edge_cases/python_rules/violations/providers/stock_feed.py new file mode 100644 index 000000000..f12b5b36c --- /dev/null +++ b/benchmarks/duckshop/edge_cases/python_rules/violations/providers/stock_feed.py @@ -0,0 +1 @@ +"""A provider module directly under providers/.""" diff --git a/crates/sqlbuild-pysource/src/node_units/_helpers/functions.rs b/crates/sqlbuild-pysource/src/node_units/_helpers/functions.rs index 934d7ac58..7d82ac38c 100644 --- a/crates/sqlbuild-pysource/src/node_units/_helpers/functions.rs +++ b/crates/sqlbuild-pysource/src/node_units/_helpers/functions.rs @@ -173,7 +173,7 @@ fn union_names(annotation: &Expr) -> Vec { } /// The last name of a name, an attribute chain, or the subscripted base of either. -fn last_name(expression: &Expr) -> Option { +pub(crate) fn last_name(expression: &Expr) -> Option { match expression { Expr::Name(name) => Some(name.id.to_string()), Expr::Attribute(attribute) => Some(attribute.attr.to_string()), diff --git a/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs b/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs index 1ad582ceb..a7ba73d0a 100644 --- a/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs +++ b/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs @@ -1,14 +1,14 @@ //! Read a module under `python/` for the node unit rules, without importing it. -use ruff_python_ast::Stmt; +use ruff_python_ast::{Expr, Stmt}; use ruff_text_size::Ranged; use crate::cpython_tree::main::parse_cpython_module::parse_cpython_module; use crate::cpython_tree::models::ParsedSource; -use crate::node_units::_helpers::functions::{leading_docstring, unit_function}; +use crate::node_units::_helpers::functions::{last_name, leading_docstring, unit_function}; use crate::node_units::_helpers::imports::{is_type_checking_imports, module_imports}; use crate::node_units::_helpers::positions::{UnitText, unit_position}; -use crate::node_units::models::{UnitModule, UnitStatement, UnitStatementKind}; +use crate::node_units::models::{UnitClass, UnitModule, UnitStatement, UnitStatementKind}; /// The top-level statements and imports of `contents`, or `None` when it does not parse. #[must_use] @@ -32,6 +32,17 @@ pub fn node_unit_module(contents: &str, python_version: (u8, u8)) -> Option { UnitStatementKind::Function(unit_function(function, &source)) } + Stmt::ClassDef(class) => UnitStatementKind::Class(UnitClass { + name: class.name.to_string(), + position: unit_position(&source, class.name.range().start()), + bases: class.bases().iter().filter_map(last_name).collect(), + }), + Stmt::Assign(assign) => { + UnitStatementKind::Assignment(assign.targets.iter().filter_map(target_name).collect()) + } + Stmt::AnnAssign(assign) => { + UnitStatementKind::Assignment(target_name(&assign.target).into_iter().collect()) + } _ => UnitStatementKind::Other, }, position: unit_position(&source, statement.range().start()), @@ -42,3 +53,11 @@ pub fn node_unit_module(contents: &str, python_version: (u8, u8)) -> Option Option { + match target { + Expr::Name(name) => Some(name.id.to_string()), + _ => None, + } +} diff --git a/crates/sqlbuild-pysource/src/node_units/models.rs b/crates/sqlbuild-pysource/src/node_units/models.rs index fb2ddaa36..5c6e066fa 100644 --- a/crates/sqlbuild-pysource/src/node_units/models.rs +++ b/crates/sqlbuild-pysource/src/node_units/models.rs @@ -29,10 +29,22 @@ pub enum UnitStatementKind { /// An import, or an `if TYPE_CHECKING:` block that only imports. Import, Function(UnitFunction), - /// Any other statement, such as a class, an assignment or a loop. + Class(UnitClass), + /// An assignment, annotated or not, by the names it binds. + Assignment(Vec), + /// Any other statement, such as a loop or a call. Other, } +/// A class and the last name of each of its bases. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct UnitClass { + pub name: String, + /// Where the class's name is written. + pub position: UnitPosition, + pub bases: Vec, +} + /// One imported name of an `import` or `from ... import` at module level. #[derive(Clone, Debug, PartialEq, Eq)] pub struct UnitImport { diff --git a/crates/sqlbuild-rules/src/engine/tests/test_incremental_rules.rs b/crates/sqlbuild-rules/src/engine/tests/test_incremental_rules.rs index 1ddcca0d0..e6eee93ec 100644 --- a/crates/sqlbuild-rules/src/engine/tests/test_incremental_rules.rs +++ b/crates/sqlbuild-rules/src/engine/tests/test_incremental_rules.rs @@ -59,6 +59,9 @@ const REVIEWED_SCOPES: &[(&str, RuleScope)] = &[ ("SQBRPYTHON010", RuleScope::Python), ("SQBRPYTHON011", RuleScope::Python), ("SQBRPYTHON012", RuleScope::Python), + ("SQBRPYTHON013", RuleScope::Python), + ("SQBRPYTHON014", RuleScope::Python), + ("SQBRPYTHON015", RuleScope::Python), ("SQBRPYTHON101", RuleScope::Python), ("SQBRPYTHON102", RuleScope::Python), ("SQBRPYTHON103", RuleScope::Python), diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/mod.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/mod.rs index 86a8cf301..4be254067 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/units/mod.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/mod.rs @@ -3,3 +3,4 @@ pub(crate) mod layout; pub(crate) mod modules; pub(crate) mod nodes; pub(crate) mod project; +pub(crate) mod providers; diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/modules.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/modules.rs index 07a5d825b..7737766bb 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/units/modules.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/modules.rs @@ -53,7 +53,7 @@ fn node_module_faults(file: &UnitFile<'_>) -> Vec { function.position, ), }, - UnitStatementKind::Other => (OTHER_STATEMENT.to_owned(), statement.position), + UnitStatementKind::Class(_) | UnitStatementKind::Assignment(_) | UnitStatementKind::Other => (OTHER_STATEMENT.to_owned(), statement.position), }; faults.extend(unit_fault( NODE_MODULE_SHAPE_CODE, @@ -95,7 +95,7 @@ fn main_module_faults(file: &UnitFile<'_>, project: &UnitProject<'_>) -> Vec (OTHER_STATEMENT.to_owned(), statement.position), + UnitStatementKind::Class(_) | UnitStatementKind::Assignment(_) | UnitStatementKind::Other => (OTHER_STATEMENT.to_owned(), statement.position), }; faults.extend(unit_fault( MAIN_MODULE_SHAPE_CODE, @@ -208,7 +208,7 @@ fn checks_module_faults(file: &UnitFile<'_>, project: &UnitProject<'_>) -> Vec { + UnitStatementKind::Class(_) | UnitStatementKind::Assignment(_) | UnitStatementKind::Other => { vec![(OTHER_STATEMENT.to_owned(), statement.position)] } }; diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/project.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/project.rs index ff3c6791b..8299d9621 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/units/project.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/project.rs @@ -107,7 +107,7 @@ fn nearest_unit(parts: &[&str], units: &BTreeSet) -> Option { } /// The dotted module of a Python path and of every package above it. -fn dotted_names(path: &str) -> Vec { +pub(crate) fn dotted_names(path: &str) -> Vec { let stem: &str = path.strip_suffix(".py").unwrap_or(path); let parts: Vec<&str> = stem .split('/') @@ -207,11 +207,21 @@ pub(crate) fn imported_module( import: &UnitImport, file: &UnitFile<'_>, project: &UnitProject<'_>, +) -> Option { + let mut folder: Vec<&str> = vec![PACKAGE_ROOT]; + folder.extend(&file.parts[..file.parts.len() - 1]); + absolute_module(import, &folder, &project.modules) +} + +/// The absolute dotted module `import` reads from a file in the project folder `folder`, given every project module. +pub(crate) fn absolute_module( + import: &UnitImport, + folder: &[&str], + modules: &BTreeSet, ) -> Option { let mut base: Vec<&str> = Vec::new(); if import.level > 0 { - base.push(PACKAGE_ROOT); - base.extend(&file.parts[..file.parts.len() - 1]); + base.extend(folder); for _ in 1..import.level { base.pop()?; } @@ -222,7 +232,7 @@ pub(crate) fn imported_module( return Some(module); }; let submodule: String = format!("{module}.{name}"); - Some(if project.modules.contains(&submodule) { + Some(if modules.contains(&submodule) { submodule } else { module diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs new file mode 100644 index 000000000..b3d1f66cf --- /dev/null +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs @@ -0,0 +1,226 @@ +//! Provider capability folders: what they hold, what provider.py declares, and who imports their private code. + +use std::collections::BTreeSet; + +use sqlbuild_pysource::node_units::main::node_unit_module::node_unit_module; +use sqlbuild_pysource::node_units::models::{UnitClass, UnitModule, UnitPosition, UnitStatementKind}; + +use crate::models::{Fault, PythonSource}; +use crate::python_rules::_helpers::faults::catalogue_fault; +use crate::python_rules::_helpers::placement::package_anchor; +use crate::python_rules::_helpers::units::project::{absolute_module, dotted_names, unit_fault}; +use crate::python_rules::constants::{ + INIT_FILE, ORCHESTRATION_ROOT, PACKAGE_ROOT, PRIVATE_PREFIX, PROVIDER_BASE, PROVIDER_FILE, + PROVIDER_IMPORT_BOUNDARIES_CODE, PROVIDER_LAYOUT_CODE, PROVIDER_MODULE_SHAPE_CODE, + PROVIDER_PRIVATE_FOLDERS, PROVIDER_ROLE_FILES, PROVIDER_RULE_CODES, PROVIDERS_ROOT, + SHARED_UTILITIES_PACKAGE, +}; + +const FILE_START: UnitPosition = UnitPosition { line: 1, column: 1 }; + +/// One parsed project file and its path parts. +#[derive(Debug)] +struct ParsedFile<'a> { + path: &'a str, + parts: Vec<&'a str>, + module: UnitModule, +} + +/// Faults of the selected provider rules among `codes`. +pub(crate) fn provider_faults( + sources: &[PythonSource], + codes: &BTreeSet<&str>, + python_version: (u8, u8), +) -> Vec { + if !PROVIDER_RULE_CODES.iter().any(|code| codes.contains(code)) { + return Vec::new(); + } + let mut faults: Vec = provider_layout_faults(sources); + let files: Vec> = sources + .iter() + .filter_map(|source| { + Some(ParsedFile { + path: source.relative_path.as_str(), + parts: source.relative_path.split('/').collect(), + module: node_unit_module(&source.text, python_version)?, + }) + }) + .collect(); + let modules: BTreeSet = sources + .iter() + .flat_map(|source| dotted_names(&source.relative_path)) + .collect(); + for file in &files { + if matches!(file.parts.as_slice(), [root, _, file_name] if *root == PROVIDERS_ROOT && *file_name == PROVIDER_FILE) + { + faults.extend(provider_module_faults(file)); + } + faults.extend(import_faults(file, &modules)); + } + faults.retain(|fault| codes.contains(fault.code.as_str())); + faults +} + +/// Modules directly in providers/, capability folders without provider.py, and modules or folders they may not hold. +fn provider_layout_faults(sources: &[PythonSource]) -> Vec { + let mut faults: Vec = Vec::new(); + let mut capabilities: BTreeSet<&str> = BTreeSet::new(); + let mut folders: BTreeSet = BTreeSet::new(); + for source in sources { + let path: &str = source.relative_path.as_str(); + let Some(rest) = path.strip_prefix(&format!("{PROVIDERS_ROOT}/")) else { + continue; + }; + let parts: Vec<&str> = rest.split('/').collect(); + let detail: Option = match parts.as_slice() { + [file] if *file == INIT_FILE => None, + [_] => Some(format!("{path} sits directly in {PROVIDERS_ROOT}/")), + [capability, file] => { + capabilities.insert(capability); + (*file != PROVIDER_FILE && !PROVIDER_ROLE_FILES.contains(file)) + .then(|| format!("{path} is not provider.py or a provider role file")) + } + [capability, folder, ..] => { + capabilities.insert(capability); + if !PROVIDER_PRIVATE_FOLDERS.contains(folder) { + folders.insert(format!("{PROVIDERS_ROOT}/{capability}/{folder}")); + } + None + } + [] => None, + }; + if let Some(detail) = detail { + faults.extend(catalogue_fault(PROVIDER_LAYOUT_CODE, path, (1, 1), &detail)); + } + } + let paths: BTreeSet<&str> = sources + .iter() + .map(|source| source.relative_path.as_str()) + .collect(); + faults.extend( + capabilities + .iter() + .map(|capability| format!("{PROVIDERS_ROOT}/{capability}")) + .filter(|folder| !paths.contains(format!("{folder}/{PROVIDER_FILE}").as_str())) + .filter_map(|folder| { + catalogue_fault( + PROVIDER_LAYOUT_CODE, + package_anchor(sources, &folder)?, + (1, 1), + &format!("{folder}/ has no {PROVIDER_FILE}"), + ) + }), + ); + faults.extend(folders.iter().filter_map(|folder| { + catalogue_fault( + PROVIDER_LAYOUT_CODE, + package_anchor(sources, folder)?, + (1, 1), + &format!("{folder}/ is not _helpers/ or _classes/"), + ) + })); + faults +} + +/// provider.py: imports, private constants and exactly one public Provider subclass. +fn provider_module_faults(file: &ParsedFile<'_>) -> Vec { + let mut faults: Vec = Vec::new(); + let mut providers: usize = 0; + for statement in &file.module.statements { + let (detail, position): (String, UnitPosition) = match &statement.kind { + UnitStatementKind::Docstring | UnitStatementKind::Import => continue, + UnitStatementKind::Assignment(names) => { + match names.iter().find(|name| !name.starts_with(PRIVATE_PREFIX)) { + Some(name) => ( + format!("public constant '{name}' belongs in constants.py"), + statement.position, + ), + None => continue, + } + } + UnitStatementKind::Class(class) => match class_detail(class, &mut providers) { + Some(detail) => (detail, class.position), + None => continue, + }, + UnitStatementKind::Function(function) => ( + format!("function '{}' belongs in _helpers/", function.name), + function.position, + ), + UnitStatementKind::Other => ( + "a statement that is not an import, a constant or the provider class".to_owned(), + statement.position, + ), + }; + faults.extend(unit_fault(PROVIDER_MODULE_SHAPE_CODE, file.path, position, &detail)); + } + if providers == 0 { + faults.extend(unit_fault( + PROVIDER_MODULE_SHAPE_CODE, + file.path, + FILE_START, + &format!("{} declares no public {PROVIDER_BASE} subclass", file.path), + )); + } + faults +} + +/// Why `class` does not belong in provider.py, counting public Provider subclasses in `providers`. +fn class_detail(class: &UnitClass, providers: &mut usize) -> Option { + let name: &str = &class.name; + if name.starts_with(PRIVATE_PREFIX) { + return Some(format!("collaborator class '{name}' belongs in _classes/")); + } + if !class.bases.iter().any(|base| base == PROVIDER_BASE) { + return Some(format!("class '{name}' does not subclass {PROVIDER_BASE}")); + } + *providers += 1; + (*providers > 1).then(|| format!("'{name}' is a second {PROVIDER_BASE} subclass")) +} + +/// Imports of another provider's private code, and a provider's imports of python/ outside utils/ or of orchestration/. +fn import_faults(file: &ParsedFile<'_>, modules: &BTreeSet) -> Vec { + let capability: Option<&str> = match file.parts.as_slice() { + [root, capability, _, ..] if *root == PROVIDERS_ROOT => Some(capability), + _ => None, + }; + let folder: &[&str] = &file.parts[..file.parts.len() - 1]; + file.module + .imports + .iter() + .filter_map(|import| { + let module: String = absolute_module(import, folder, modules)?; + if !modules.contains(&module) { + return None; + } + let detail: String = boundary_detail(&module, capability)?; + unit_fault( + PROVIDER_IMPORT_BOUNDARIES_CODE, + file.path, + import.position, + &format!("{} imports {module}, {detail}", file.path), + ) + }) + .collect() +} + +/// Why a file in provider `capability` (or outside providers/ when `None`) may not import `module`. +fn boundary_detail(module: &str, capability: Option<&str>) -> Option { + let parts: Vec<&str> = module.split('.').collect(); + if let [root, owner, folder, ..] = parts.as_slice() { + let private: bool = *root == PROVIDERS_ROOT && PROVIDER_PRIVATE_FOLDERS.contains(folder); + if private && capability != Some(*owner) { + return Some(format!("which is private to {PROVIDERS_ROOT}/{owner}/")); + } + } + capability?; + match parts.as_slice() { + [root, utils, ..] if *root == PACKAGE_ROOT && *utils == SHARED_UTILITIES_PACKAGE => None, + [root, ..] if *root == PACKAGE_ROOT => Some(format!( + "but a provider imports only {PACKAGE_ROOT}/{SHARED_UTILITIES_PACKAGE}/ from {PACKAGE_ROOT}/" + )), + [root, ..] if *root == ORCHESTRATION_ROOT => { + Some(format!("but a provider never imports {ORCHESTRATION_ROOT}/")) + } + _ => None, + } +} diff --git a/crates/sqlbuild-rules/src/python_rules/constants.rs b/crates/sqlbuild-rules/src/python_rules/constants.rs index 114a17029..a7cfe9245 100644 --- a/crates/sqlbuild-rules/src/python_rules/constants.rs +++ b/crates/sqlbuild-rules/src/python_rules/constants.rs @@ -5,7 +5,7 @@ use fensu_native::rules::constants as fensu; use crate::python_rules::models::{PythonFiles, PythonRuleSpec}; /// Every `SQBRPYTHON` rule; numbers are never reused, and new rules append to their band. -pub(crate) const PYTHON_RULES: [PythonRuleSpec; 74] = [ +pub(crate) const PYTHON_RULES: [PythonRuleSpec; 77] = [ PythonRuleSpec { code: "SQBRPYTHON001", fensu_code: None, @@ -150,6 +150,42 @@ pub(crate) const PYTHON_RULES: [PythonRuleSpec; 74] = [ message: "nodes are declared in a unit's node.py, and checks in a unit's checks.py", remediation: "Move the declaration into its own unit folder under its domain, with the implementation in that folder's main.py.", }, + PythonRuleSpec { + code: "SQBRPYTHON013", + fensu_code: None, + slug: "provider-layout", + family: "providers", + owner: "file", + applies: PythonFiles::All, + enabled_by_default: true, + options: &[], + message: "providers/ holds capability folders, each with provider.py, optional role files, _helpers/ and _classes/ only", + remediation: "Move a module directly under providers/ into providers//provider.py, add provider.py to each capability folder, and move other modules into _helpers/, _classes/ or a role file: models.py, types.py, constants.py or exceptions.py.", + }, + PythonRuleSpec { + code: "SQBRPYTHON014", + fensu_code: None, + slug: "provider-module-shape", + family: "providers", + owner: "file", + applies: PythonFiles::All, + enabled_by_default: true, + options: &[], + message: "provider.py holds imports, private constants and exactly one public Provider subclass", + remediation: "Keep one public class that subclasses Provider in provider.py; move functions into _helpers/, collaborator classes into _classes/ and public constants into constants.py.", + }, + PythonRuleSpec { + code: "SQBRPYTHON015", + fensu_code: None, + slug: "provider-import-boundaries", + family: "providers", + owner: "file", + applies: PythonFiles::All, + enabled_by_default: true, + options: &[], + message: "a provider's _helpers/ and _classes/ are private to it, and providers import only python/utils/ from the runtime package and nothing from orchestration/", + remediation: "Import a provider through provider.py or its role files; move code a provider shares with nodes into python/utils//.", + }, PythonRuleSpec { code: "SQBRPYTHON101", fensu_code: Some(fensu::PARAMETER_ANNOTATION_CODE), @@ -1036,6 +1072,33 @@ pub(crate) const NODE_DESCRIPTION_CODE: &str = "SQBRPYTHON011"; pub(crate) const NODE_DECLARED_IN_UNIT_CODE: &str = "SQBRPYTHON012"; /// The fewest folders below `python/` a unit sits in: its domain and its own. pub(crate) const MIN_UNIT_DEPTH: usize = 2; +pub(crate) const PROVIDER_LAYOUT_CODE: &str = "SQBRPYTHON013"; +pub(crate) const PROVIDER_MODULE_SHAPE_CODE: &str = "SQBRPYTHON014"; +pub(crate) const PROVIDER_IMPORT_BOUNDARIES_CODE: &str = "SQBRPYTHON015"; +/// The provider rules, which read parsed modules across the project. +pub(crate) const PROVIDER_RULE_CODES: [&str; 3] = [ + PROVIDER_LAYOUT_CODE, + PROVIDER_MODULE_SHAPE_CODE, + PROVIDER_IMPORT_BOUNDARIES_CODE, +]; +/// The file declaring a provider capability's Provider subclass. +pub(crate) const PROVIDER_FILE: &str = "provider.py"; +/// The files a provider capability folder may hold beside provider.py. +pub(crate) const PROVIDER_ROLE_FILES: [&str; 5] = [ + "models.py", + "types.py", + "constants.py", + "exceptions.py", + INIT_FILE, +]; +/// The private folders a provider capability folder may hold. +pub(crate) const PROVIDER_PRIVATE_FOLDERS: [&str; 2] = ["_helpers", "_classes"]; +/// The base class a provider subclasses. +pub(crate) const PROVIDER_BASE: &str = "Provider"; +/// The folder holding orchestration declarations, imported as `orchestration`. +pub(crate) const ORCHESTRATION_ROOT: &str = "orchestration"; +/// The prefix of a private name. +pub(crate) const PRIVATE_PREFIX: &str = "_"; /// The file that declares a unit's node and makes its folder a unit. pub(crate) const UNIT_NODE_FILE: &str = "node.py"; /// The file holding a unit's checks. diff --git a/crates/sqlbuild-rules/src/python_rules/main/evaluate_python_rules.rs b/crates/sqlbuild-rules/src/python_rules/main/evaluate_python_rules.rs index 0ebea79d5..103cc7fe3 100644 --- a/crates/sqlbuild-rules/src/python_rules/main/evaluate_python_rules.rs +++ b/crates/sqlbuild-rules/src/python_rules/main/evaluate_python_rules.rs @@ -13,6 +13,7 @@ use crate::python_rules::_helpers::engine::placement::python_targets; use crate::python_rules::_helpers::engine::programs::Programs; use crate::python_rules::_helpers::engine::project_plane::loaded_project; use crate::python_rules::_helpers::units::project::unit_faults; +use crate::python_rules::_helpers::units::providers::provider_faults; use crate::python_rules::models::{ CallInputs, PythonCacheEntry, PythonEvaluation, PythonRuleSpec, PythonTarget, }; @@ -49,6 +50,11 @@ pub(crate) fn evaluate_python_rules( &codes, request.python_version, )); + evaluation.faults.extend(provider_faults( + &request.python_sources, + &codes, + request.python_version, + )); evaluation.faults.sort_by(|left, right| { (&left.path, left.line, left.column, &left.code).cmp(&( &right.path, diff --git a/crates/sqlbuild-rules/src/python_rules/tests.rs b/crates/sqlbuild-rules/src/python_rules/tests.rs index 0506832c2..ae23fb200 100644 --- a/crates/sqlbuild-rules/src/python_rules/tests.rs +++ b/crates/sqlbuild-rules/src/python_rules/tests.rs @@ -4,6 +4,8 @@ mod evaluate_python_rules; mod helpers; #[path = "tests/test_node_units.rs"] mod node_units; +#[path = "tests/test_provider_rules.rs"] +mod provider_rules; #[path = "tests/test_python_cache.rs"] mod python_cache; #[path = "tests/test_python_catalogue.rs"] diff --git a/crates/sqlbuild-rules/src/python_rules/tests/test_provider_rules.rs b/crates/sqlbuild-rules/src/python_rules/tests/test_provider_rules.rs new file mode 100644 index 000000000..a75d76ca9 --- /dev/null +++ b/crates/sqlbuild-rules/src/python_rules/tests/test_provider_rules.rs @@ -0,0 +1,149 @@ +use serde_json::json; + +use crate::python_rules::tests::helpers::{owned_faults, python_rule_faults}; +use crate::python_rules::tests::test_types::PythonRunTestCase; + +const PROVIDER_RULES: [&str; 3] = ["SQBRPYTHON013", "SQBRPYTHON014", "SQBRPYTHON015"]; + +#[test] +fn given_providers_when_evaluating_provider_rules_then_reports_each_broken_convention() { + let test_cases = [ + PythonRunTestCase { + description: "capability folders with provider.py, role files and private folders report nothing", + files: &[ + ("providers/__init__.py", ""), + ( + "providers/storefront_api/provider.py", + r#""""Storefront client.""" +from sqlbuild.providers import Provider + +from python.utils.retries import ATTEMPTS + +from ._classes.page_cursor import PageCursor + +_PAGE_SIZE: int = 100 + + +class StorefrontApi(Provider): + """Read-only client for the storefront order API.""" + + base_url: str +"#, + ), + ("providers/storefront_api/models.py", ""), + ("providers/storefront_api/_classes/page_cursor.py", ""), + ("providers/storefront_api/_helpers/paging.py", ""), + ("python/utils/retries.py", ""), + ( + "python/orders/daily/main.py", + "from providers.storefront_api.provider import StorefrontApi\n", + ), + ], + config: json!({ "select": PROVIDER_RULES }), + expected_faults: vec![], + }, + PythonRunTestCase { + description: "layout: a module directly in providers/, a stray module, a missing provider.py and a stray folder", + files: &[ + ("providers/storefront_api.py", ""), + ("providers/carrier_api/client.py", ""), + ("providers/partner_sftp/provider.py", ""), + ("providers/partner_sftp/sftp/connection.py", ""), + ], + config: json!({ "select": ["SQBRPYTHON013"] }), + expected_faults: vec![ + ("SQBRPYTHON013", "providers/carrier_api/client.py", 1, 1), + ("SQBRPYTHON013", "providers/carrier_api/client.py", 1, 1), + ("SQBRPYTHON013", "providers/partner_sftp/sftp/connection.py", 1, 1), + ("SQBRPYTHON013", "providers/storefront_api.py", 1, 1), + ], + }, + PythonRunTestCase { + description: "provider.py: a public constant, a function, a private and a plain class, a second provider, and none at all", + files: &[ + ( + "providers/storefront_api/provider.py", + r#""""Doc.""" +from sqlbuild.providers import Provider + +_TIMEOUT: int = 3 +RETRIES = 2 + + +def build() -> None: + pass + + +class _Cursor: + pass + + +class Helper: + pass + + +class StorefrontApi(Provider): + """Client.""" + + +class CarrierApi(Provider): + """Second.""" +"#, + ), + ("providers/empty/provider.py", "import os\n"), + ], + config: json!({ "select": ["SQBRPYTHON014"] }), + expected_faults: vec![ + ("SQBRPYTHON014", "providers/empty/provider.py", 1, 1), + ("SQBRPYTHON014", "providers/storefront_api/provider.py", 5, 1), + ("SQBRPYTHON014", "providers/storefront_api/provider.py", 8, 5), + ("SQBRPYTHON014", "providers/storefront_api/provider.py", 12, 7), + ("SQBRPYTHON014", "providers/storefront_api/provider.py", 16, 7), + ("SQBRPYTHON014", "providers/storefront_api/provider.py", 24, 7), + ], + }, + PythonRunTestCase { + description: "imports: another provider's private code from anywhere, and python/ outside utils/ or orchestration/ from a provider", + files: &[ + ( + "providers/storefront_api/provider.py", + r#"from python.utils.retries import ATTEMPTS +from python.orders.daily.main import main +from orchestration.sensors.orders import on_orders +from providers.carrier_api._helpers.auth import token +from ._classes.cursor import Cursor +from providers.carrier_api.provider import CarrierApi +"#, + ), + ("providers/storefront_api/_classes/cursor.py", ""), + ("providers/carrier_api/provider.py", ""), + ("providers/carrier_api/_helpers/auth.py", ""), + ("python/utils/retries.py", ""), + ( + "python/orders/daily/main.py", + "from providers.storefront_api._classes.cursor import Cursor\n", + ), + ("orchestration/sensors/orders.py", ""), + ("hooks/python/notify.py", "import providers.carrier_api._helpers.auth\n"), + ], + config: json!({ "select": ["SQBRPYTHON015"] }), + expected_faults: vec![ + ("SQBRPYTHON015", "hooks/python/notify.py", 1, 8), + ("SQBRPYTHON015", "providers/storefront_api/provider.py", 2, 38), + ("SQBRPYTHON015", "providers/storefront_api/provider.py", 3, 42), + ("SQBRPYTHON015", "providers/storefront_api/provider.py", 4, 49), + ("SQBRPYTHON015", "python/orders/daily/main.py", 1, 54), + ], + }, + ]; + for test_case in test_cases { + let faults = python_rule_faults(test_case.files, test_case.config.clone()); + + assert_eq!( + faults, + Ok(owned_faults(&test_case.expected_faults)), + "{}", + test_case.description + ); + } +} diff --git a/crates/sqlbuild-rules/src/python_rules/tests/test_python_catalogue.rs b/crates/sqlbuild-rules/src/python_rules/tests/test_python_catalogue.rs index f8be29887..95ce1c8d8 100644 --- a/crates/sqlbuild-rules/src/python_rules/tests/test_python_catalogue.rs +++ b/crates/sqlbuild-rules/src/python_rules/tests/test_python_catalogue.rs @@ -14,14 +14,14 @@ use crate::rules::models::BuiltinOptionKind; fn given_the_catalogue_when_listing_python_rules_then_each_has_its_own_project_wide_code() { let test_cases = [ CatalogueTestCase { - description: "12 SQLBuild layout and unit rules and 62 Fensu core rules have distinct codes", + description: "15 SQLBuild layout, unit and provider rules and 62 Fensu core rules have distinct codes", count: distinct_python_codes, - expected_count: 74, + expected_count: 77, }, CatalogueTestCase { description: "every Python rule reports against the project in the python family", count: project_wide_python_rules, - expected_count: 74, + expected_count: 77, }, ]; for test_case in test_cases { diff --git a/website/src/content/docs/docs/concepts/python-nodes/providers.mdx b/website/src/content/docs/docs/concepts/python-nodes/providers.mdx index 770ff28ad..ecdbd805d 100644 --- a/website/src/content/docs/docs/concepts/python-nodes/providers.mdx +++ b/website/src/content/docs/docs/concepts/python-nodes/providers.mdx @@ -216,3 +216,7 @@ my-project/ tasks/ export_orders.py ``` + +A project that selects the [Python Rules](/docs/concepts/rules/python-rules#laying-out-providers) +keeps each provider in its own folder, `providers//provider.py`, and each node in its +own unit folder under its domain. diff --git a/website/src/content/docs/docs/concepts/rules/python-rules.mdx b/website/src/content/docs/docs/concepts/rules/python-rules.mdx index a6d101d2e..6e19045de 100644 --- a/website/src/content/docs/docs/concepts/rules/python-rules.mdx +++ b/website/src/content/docs/docs/concepts/rules/python-rules.mdx @@ -145,6 +145,31 @@ A unit may import another unit's `node.py` only to name that node in `depends_on A factory's `node.py` holds one `@factory` function, and a check generated by a factory may stay in `node.py`. `@audit_factory` functions are not units and may sit anywhere under `python/`. +## Laying out providers + +`SQBRPYTHON013` to `SQBRPYTHON015` keep each provider in its own capability folder, with its +private code beside it: + +```text +providers/ + storefront_api/ + provider.py # the one public Provider subclass + models.py + _classes/ + page_cursor.py # a private collaborator class + carrier_api/ + provider.py +``` + +A capability folder may also hold `types.py`, `constants.py`, `exceptions.py`, `__init__.py` and +`_helpers/`. These are reported: + +| Code | Finding | Fix | +|---|---|---| +| `SQBRPYTHON013` | A module directly in `providers/`, a capability folder without `provider.py`, or another module or folder in it | Move it to `providers//provider.py`, `_helpers/`, `_classes/` or a role file | +| `SQBRPYTHON014` | `provider.py` holding a function, a public constant, another class or a second provider | Keep one public `Provider` subclass and private constants; move the rest to `_helpers/`, `_classes/` or `constants.py` | +| `SQBRPYTHON015` | An import of a provider's `_helpers/` or `_classes/` from outside its folder, or a provider importing `python/` outside `utils/` or anything in `orchestration/` | Import the provider through `provider.py` or its role files; share code through `python/utils/` | + ## Main modules and entry modules Some Rules apply only to a domain's main modules: files under a `main/` folder, and a unit's @@ -226,6 +251,14 @@ remediation. | `SQBRPYTHON011` | node-description-not-docstring | `python/` | yes | Tasks, assets and checks set `description=` without a docstring; loaders set neither | | `SQBRPYTHON012` | node-declared-in-unit | `python/` | yes | Nodes are declared in a unit's `node.py`, and checks in its `checks.py` | +### Providers + +| Code | Name | Files | Default | Checks | +|---|---|---|---|---| +| `SQBRPYTHON013` | provider-layout | All | yes | `providers/` holds capability folders with `provider.py`, role files, `_helpers/` and `_classes/` only | +| `SQBRPYTHON014` | provider-module-shape | All | yes | `provider.py` holds imports, private constants and one public `Provider` subclass | +| `SQBRPYTHON015` | provider-import-boundaries | All | yes | A provider's `_helpers/` and `_classes/` are private to it; providers import only `python/utils/` from `python/` and nothing from `orchestration/` | + ### Type annotations | Code | Name | Files | Default | Checks | From e34468b9620b7d634232bdbef643e9769adb396a Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sun, 15 Nov 2026 02:36:39 +0000 Subject: [PATCH 2/8] wip: provider draft builds on the reviewed unit rules --- .../src/node_units/main/node_unit_module.rs | 6 +- .../node_units/tests/test_node_unit_module.rs | 9 ++- .../python_rules/_helpers/units/modules.rs | 12 +++- .../python_rules/_helpers/units/providers.rs | 21 ++++-- .../python_rules/tests/test_provider_rules.rs | 68 ++++++++++++++++--- 5 files changed, 91 insertions(+), 25 deletions(-) diff --git a/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs b/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs index a7ba73d0a..0c320bf51 100644 --- a/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs +++ b/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs @@ -37,9 +37,9 @@ pub fn node_unit_module(contents: &str, python_version: (u8, u8)) -> Option { - UnitStatementKind::Assignment(assign.targets.iter().filter_map(target_name).collect()) - } + Stmt::Assign(assign) => UnitStatementKind::Assignment( + assign.targets.iter().filter_map(target_name).collect(), + ), Stmt::AnnAssign(assign) => { UnitStatementKind::Assignment(target_name(&assign.target).into_iter().collect()) } diff --git a/crates/sqlbuild-pysource/src/node_units/tests/test_node_unit_module.rs b/crates/sqlbuild-pysource/src/node_units/tests/test_node_unit_module.rs index 18c4892f5..cc9f74360 100644 --- a/crates/sqlbuild-pysource/src/node_units/tests/test_node_unit_module.rs +++ b/crates/sqlbuild-pysource/src/node_units/tests/test_node_unit_module.rs @@ -14,17 +14,22 @@ const FACTORY_MODULE: &str = "@factory\ndef tables():\n for table in TABLES:\ fn given_module_when_reading_then_each_top_level_statement_has_its_kind() { let test_cases = [ StatementKindsTestCase { - description: "a CRLF node module with a docstring, imports, a value and a node", + description: "a CRLF node module with a docstring, imports, an assignment and a node", contents: CRLF_NODE_MODULE, expected_kinds: Some(vec![ "Docstring", "Import", "Import", "Import", - "Other", + "Assignment", "Function", ]), }, + StatementKindsTestCase { + description: "a class and a loop", + contents: "class Store(Provider):\n pass\n\n\nfor row in rows:\n pass\n", + expected_kinds: Some(vec!["Class", "Other"]), + }, StatementKindsTestCase { description: "a module that does not parse", contents: "def broken(:\n", diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/modules.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/modules.rs index 7737766bb..2f9f4de65 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/units/modules.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/modules.rs @@ -53,7 +53,9 @@ fn node_module_faults(file: &UnitFile<'_>) -> Vec { function.position, ), }, - UnitStatementKind::Class(_) | UnitStatementKind::Assignment(_) | UnitStatementKind::Other => (OTHER_STATEMENT.to_owned(), statement.position), + UnitStatementKind::Class(_) + | UnitStatementKind::Assignment(_) + | UnitStatementKind::Other => (OTHER_STATEMENT.to_owned(), statement.position), }; faults.extend(unit_fault( NODE_MODULE_SHAPE_CODE, @@ -95,7 +97,9 @@ fn main_module_faults(file: &UnitFile<'_>, project: &UnitProject<'_>) -> Vec (OTHER_STATEMENT.to_owned(), statement.position), + UnitStatementKind::Class(_) + | UnitStatementKind::Assignment(_) + | UnitStatementKind::Other => (OTHER_STATEMENT.to_owned(), statement.position), }; faults.extend(unit_fault( MAIN_MODULE_SHAPE_CODE, @@ -208,7 +212,9 @@ fn checks_module_faults(file: &UnitFile<'_>, project: &UnitProject<'_>) -> Vec { + UnitStatementKind::Class(_) + | UnitStatementKind::Assignment(_) + | UnitStatementKind::Other => { vec![(OTHER_STATEMENT.to_owned(), statement.position)] } }; diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs index b3d1f66cf..384612bd6 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs @@ -3,11 +3,13 @@ use std::collections::BTreeSet; use sqlbuild_pysource::node_units::main::node_unit_module::node_unit_module; -use sqlbuild_pysource::node_units::models::{UnitClass, UnitModule, UnitPosition, UnitStatementKind}; +use sqlbuild_pysource::node_units::models::{ + UnitClass, UnitModule, UnitPosition, UnitStatementKind, +}; use crate::models::{Fault, PythonSource}; -use crate::python_rules::_helpers::faults::catalogue_fault; -use crate::python_rules::_helpers::placement::package_anchor; +use crate::python_rules::_helpers::engine::faults::catalogue_fault; +use crate::python_rules::_helpers::engine::placement::package_anchor; use crate::python_rules::_helpers::units::project::{absolute_module, dotted_names, unit_fault}; use crate::python_rules::constants::{ INIT_FILE, ORCHESTRATION_ROOT, PACKAGE_ROOT, PRIVATE_PREFIX, PROVIDER_BASE, PROVIDER_FILE, @@ -151,7 +153,12 @@ fn provider_module_faults(file: &ParsedFile<'_>) -> Vec { statement.position, ), }; - faults.extend(unit_fault(PROVIDER_MODULE_SHAPE_CODE, file.path, position, &detail)); + faults.extend(unit_fault( + PROVIDER_MODULE_SHAPE_CODE, + file.path, + position, + &detail, + )); } if providers == 0 { faults.extend(unit_fault( @@ -218,9 +225,9 @@ fn boundary_detail(module: &str, capability: Option<&str>) -> Option { [root, ..] if *root == PACKAGE_ROOT => Some(format!( "but a provider imports only {PACKAGE_ROOT}/{SHARED_UTILITIES_PACKAGE}/ from {PACKAGE_ROOT}/" )), - [root, ..] if *root == ORCHESTRATION_ROOT => { - Some(format!("but a provider never imports {ORCHESTRATION_ROOT}/")) - } + [root, ..] if *root == ORCHESTRATION_ROOT => Some(format!( + "but a provider never imports {ORCHESTRATION_ROOT}/" + )), _ => None, } } diff --git a/crates/sqlbuild-rules/src/python_rules/tests/test_provider_rules.rs b/crates/sqlbuild-rules/src/python_rules/tests/test_provider_rules.rs index a75d76ca9..d4bcb2659 100644 --- a/crates/sqlbuild-rules/src/python_rules/tests/test_provider_rules.rs +++ b/crates/sqlbuild-rules/src/python_rules/tests/test_provider_rules.rs @@ -54,7 +54,12 @@ class StorefrontApi(Provider): expected_faults: vec![ ("SQBRPYTHON013", "providers/carrier_api/client.py", 1, 1), ("SQBRPYTHON013", "providers/carrier_api/client.py", 1, 1), - ("SQBRPYTHON013", "providers/partner_sftp/sftp/connection.py", 1, 1), + ( + "SQBRPYTHON013", + "providers/partner_sftp/sftp/connection.py", + 1, + 1, + ), ("SQBRPYTHON013", "providers/storefront_api.py", 1, 1), ], }, @@ -95,11 +100,36 @@ class CarrierApi(Provider): config: json!({ "select": ["SQBRPYTHON014"] }), expected_faults: vec![ ("SQBRPYTHON014", "providers/empty/provider.py", 1, 1), - ("SQBRPYTHON014", "providers/storefront_api/provider.py", 5, 1), - ("SQBRPYTHON014", "providers/storefront_api/provider.py", 8, 5), - ("SQBRPYTHON014", "providers/storefront_api/provider.py", 12, 7), - ("SQBRPYTHON014", "providers/storefront_api/provider.py", 16, 7), - ("SQBRPYTHON014", "providers/storefront_api/provider.py", 24, 7), + ( + "SQBRPYTHON014", + "providers/storefront_api/provider.py", + 5, + 1, + ), + ( + "SQBRPYTHON014", + "providers/storefront_api/provider.py", + 8, + 5, + ), + ( + "SQBRPYTHON014", + "providers/storefront_api/provider.py", + 12, + 7, + ), + ( + "SQBRPYTHON014", + "providers/storefront_api/provider.py", + 16, + 7, + ), + ( + "SQBRPYTHON014", + "providers/storefront_api/provider.py", + 24, + 7, + ), ], }, PythonRunTestCase { @@ -124,14 +154,32 @@ from providers.carrier_api.provider import CarrierApi "from providers.storefront_api._classes.cursor import Cursor\n", ), ("orchestration/sensors/orders.py", ""), - ("hooks/python/notify.py", "import providers.carrier_api._helpers.auth\n"), + ( + "hooks/python/notify.py", + "import providers.carrier_api._helpers.auth\n", + ), ], config: json!({ "select": ["SQBRPYTHON015"] }), expected_faults: vec![ ("SQBRPYTHON015", "hooks/python/notify.py", 1, 8), - ("SQBRPYTHON015", "providers/storefront_api/provider.py", 2, 38), - ("SQBRPYTHON015", "providers/storefront_api/provider.py", 3, 42), - ("SQBRPYTHON015", "providers/storefront_api/provider.py", 4, 49), + ( + "SQBRPYTHON015", + "providers/storefront_api/provider.py", + 2, + 38, + ), + ( + "SQBRPYTHON015", + "providers/storefront_api/provider.py", + 3, + 42, + ), + ( + "SQBRPYTHON015", + "providers/storefront_api/provider.py", + 4, + 49, + ), ("SQBRPYTHON015", "python/orders/daily/main.py", 1, 54), ], }, From e1f753ac42d0136b1c95e1d6fb86abd395a3d8a5 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sun, 15 Nov 2026 03:01:08 +0000 Subject: [PATCH 3/8] wip: finish SQBRPYTHON provider rules: fixtures, findings, docs and counts Parse files in parallel, keep the provider count in the caller, regenerate the DuckShop edge-case findings and docs skill, and edit the providers page's layout section into shape. --- .../providers/carrier_api/constants.py | 3 ++ .../providers/carrier_api/provider.py | 10 +++--- .../python_rules/expected_findings.json | 4 ++- .../python_rules/_helpers/units/providers.rs | 30 ++++++++++------ .../docs/concepts/python-nodes/providers.md | 23 +++++++++---- .../docs/concepts/rules/python-rules.md | 34 +++++++++++++++++++ .../sqlbuild/cli/commands/main/test_rules.py | 2 +- .../docs/concepts/python-nodes/providers.mdx | 25 +++++++++----- 8 files changed, 99 insertions(+), 32 deletions(-) create mode 100644 benchmarks/duckshop/edge_cases/python_rules/conforming/providers/carrier_api/constants.py diff --git a/benchmarks/duckshop/edge_cases/python_rules/conforming/providers/carrier_api/constants.py b/benchmarks/duckshop/edge_cases/python_rules/conforming/providers/carrier_api/constants.py new file mode 100644 index 000000000..8465d8768 --- /dev/null +++ b/benchmarks/duckshop/edge_cases/python_rules/conforming/providers/carrier_api/constants.py @@ -0,0 +1,3 @@ +"""Carrier pickup constants.""" + +MINIMUM_CHARGE_CENTS: int = 250 diff --git a/benchmarks/duckshop/edge_cases/python_rules/conforming/providers/carrier_api/provider.py b/benchmarks/duckshop/edge_cases/python_rules/conforming/providers/carrier_api/provider.py index f05a049e5..ce2994ac3 100644 --- a/benchmarks/duckshop/edge_cases/python_rules/conforming/providers/carrier_api/provider.py +++ b/benchmarks/duckshop/edge_cases/python_rules/conforming/providers/carrier_api/provider.py @@ -1,9 +1,9 @@ """Carrier pickup client.""" -from python.utils.currency import amounts +from providers.carrier_api.constants import MINIMUM_CHARGE_CENTS from sqlbuild.providers import Provider -_MINIMUM_CHARGE_CENTS: int = 250 +_CENTS_PER_UNIT: int = 100 class CarrierApi(Provider): @@ -11,6 +11,6 @@ class CarrierApi(Provider): endpoint: str = "https://carrier.example.com" - def minimum_charge(self) -> str: - """Return the minimum pickup charge as text.""" - return amounts.cents_text(_MINIMUM_CHARGE_CENTS) + def minimum_charge(self) -> float: + """Return the minimum pickup charge in currency units.""" + return MINIMUM_CHARGE_CENTS / _CENTS_PER_UNIT diff --git a/benchmarks/duckshop/edge_cases/python_rules/expected_findings.json b/benchmarks/duckshop/edge_cases/python_rules/expected_findings.json index c72b412c7..64b2b5a45 100644 --- a/benchmarks/duckshop/edge_cases/python_rules/expected_findings.json +++ b/benchmarks/duckshop/edge_cases/python_rules/expected_findings.json @@ -2,6 +2,9 @@ "select": ["SQBRPYTHON", "SQBRPYTHON306"], "findings": [ ["SQBRPYTHON101", "hooks/python/alert.py", 4, 11], + ["SQBRPYTHON015", "providers/carrier_api/provider.py", 3, 43], + ["SQBRPYTHON014", "providers/carrier_api/provider.py", 7, 5], + ["SQBRPYTHON013", "providers/stock_feed.py", 1, 1], ["SQBRPYTHON613", "python/inventory/__init__.py", 1, 1], ["SQBRPYTHON002", "python/inventory/common/__init__.py", 1, 1], ["SQBRPYTHON306", "python/inventory/common/stock.py", 4, 1], @@ -10,7 +13,6 @@ ["SQBRPYTHON614", "python/inventory/replenishment.py", 1, 1], ["SQBRPYTHON012", "python/inventory/replenishment.py", 7, 5], ["SQBRPYTHON617", "python/inventory/restock/main.py", 1, 1], - ["SQBRPYTHON306", "python/inventory/restock/main.py", 4, 1], ["SQBRPYTHON306", "python/inventory/restock/main.py", 9, 1], ["SQBRPYTHON003", "python/inventory/stock_count/node.py", 1, 1], ["SQBRPYTHON004", "python/inventory/stock_count/node.py", 5, 1], diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs index 384612bd6..85a5b2fad 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs @@ -2,6 +2,8 @@ use std::collections::BTreeSet; +use rayon::iter::{IntoParallelRefIterator, ParallelIterator}; + use sqlbuild_pysource::node_units::main::node_unit_module::node_unit_module; use sqlbuild_pysource::node_units::models::{ UnitClass, UnitModule, UnitPosition, UnitStatementKind, @@ -39,7 +41,7 @@ pub(crate) fn provider_faults( } let mut faults: Vec = provider_layout_faults(sources); let files: Vec> = sources - .iter() + .par_iter() .filter_map(|source| { Some(ParsedFile { path: source.relative_path.as_str(), @@ -140,10 +142,14 @@ fn provider_module_faults(file: &ParsedFile<'_>) -> Vec { None => continue, } } - UnitStatementKind::Class(class) => match class_detail(class, &mut providers) { - Some(detail) => (detail, class.position), - None => continue, - }, + UnitStatementKind::Class(class) => { + let detail: Option = class_detail(class, providers); + providers += usize::from(is_public_provider(class)); + match detail { + Some(detail) => (detail, class.position), + None => continue, + } + } UnitStatementKind::Function(function) => ( format!("function '{}' belongs in _helpers/", function.name), function.position, @@ -171,17 +177,21 @@ fn provider_module_faults(file: &ParsedFile<'_>) -> Vec { faults } -/// Why `class` does not belong in provider.py, counting public Provider subclasses in `providers`. -fn class_detail(class: &UnitClass, providers: &mut usize) -> Option { +/// Why `class` does not belong in provider.py, after `earlier` public Provider subclasses. +fn class_detail(class: &UnitClass, earlier: usize) -> Option { let name: &str = &class.name; if name.starts_with(PRIVATE_PREFIX) { return Some(format!("collaborator class '{name}' belongs in _classes/")); } - if !class.bases.iter().any(|base| base == PROVIDER_BASE) { + if !is_public_provider(class) { return Some(format!("class '{name}' does not subclass {PROVIDER_BASE}")); } - *providers += 1; - (*providers > 1).then(|| format!("'{name}' is a second {PROVIDER_BASE} subclass")) + (earlier > 0).then(|| format!("'{name}' is a second {PROVIDER_BASE} subclass")) +} + +/// Whether `class` is a public subclass of Provider. +fn is_public_provider(class: &UnitClass) -> bool { + !class.name.starts_with(PRIVATE_PREFIX) && class.bases.iter().any(|base| base == PROVIDER_BASE) } /// Imports of another provider's private code, and a provider's imports of python/ outside utils/ or of orchestration/. diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/python-nodes/providers.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/python-nodes/providers.md index d1b9d7117..f131ceb30 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/python-nodes/providers.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/python-nodes/providers.md @@ -217,19 +217,30 @@ Setting values, defaults included, are never published. ## Project layout +Give each provider its own folder under `providers/`, named for the capability it provides: + ``` my-project/ providers/ - warehouse_client.py - slack_notifier.py + warehouse_client/ + provider.py # the Provider subclass + constants.py + _helpers/ # private to warehouse_client + slack_notifier/ + provider.py hooks/ sql/ record_notification.sql python/ notify.py python/ - loaders/ - load_orders.py - tasks/ - export_orders.py + orders/ + load_orders/ + node.py + main.py ``` + +Discovery also accepts `.py` files directly in `providers/`. A project that selects the +[Python Rules](../rules/python-rules.md#laying-out-providers) reports them, along with +anything else in a provider folder besides `provider.py`, its role files, `_helpers/` and +`_classes/`. diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/python-rules.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/python-rules.md index 60562d761..eb807414e 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/python-rules.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/python-rules.md @@ -12,6 +12,7 @@ Online: https://sqlbuild.com/docs/concepts/rules/python-rules/ - Which files are checked - Laying out `python/` - Writing nodes as units +- Laying out providers - Main modules and entry modules - Unreachable definitions - Rule options @@ -159,6 +160,31 @@ A unit may import another unit's `node.py` only to name that node in `depends_on A factory's `node.py` holds one `@factory` function, and a check generated by a factory may stay in `node.py`. `@audit_factory` functions are not units and may sit anywhere under `python/`. +## Laying out providers + +`SQBRPYTHON013` to `SQBRPYTHON015` keep each provider in its own capability folder, with its +private code beside it: + +```text +providers/ + storefront_api/ + provider.py # the one public Provider subclass + models.py + _classes/ + page_cursor.py # a private collaborator class + carrier_api/ + provider.py +``` + +A capability folder may also hold `types.py`, `constants.py`, `exceptions.py`, `__init__.py` and +`_helpers/`. These are reported: + +| Code | Finding | Fix | +|---|---|---| +| `SQBRPYTHON013` | A module directly in `providers/`, a capability folder without `provider.py`, or another module or folder in it | Move it to `providers//provider.py`, `_helpers/`, `_classes/` or a role file | +| `SQBRPYTHON014` | `provider.py` holding a function, a public constant, another class or a second provider | Keep one public `Provider` subclass and private constants; move the rest to `_helpers/`, `_classes/` or `constants.py` | +| `SQBRPYTHON015` | An import of a provider's `_helpers/` or `_classes/` from outside its folder, or a provider importing `python/` outside `utils/` or anything in `orchestration/` | Import the provider through `provider.py` or its role files; share code through `python/utils/` | + ## Main modules and entry modules Some Rules apply only to a domain's main modules: files under a `main/` folder, and a unit's @@ -240,6 +266,14 @@ remediation. | `SQBRPYTHON011` | node-description-not-docstring | `python/` | yes | Tasks, assets and checks set `description=` without a docstring; loaders set neither | | `SQBRPYTHON012` | node-declared-in-unit | `python/` | yes | Nodes are declared in a unit's `node.py`, and checks in its `checks.py` | +### Providers + +| Code | Name | Files | Default | Checks | +|---|---|---|---|---| +| `SQBRPYTHON013` | provider-layout | All | yes | `providers/` holds capability folders with `provider.py`, role files, `_helpers/` and `_classes/` only | +| `SQBRPYTHON014` | provider-module-shape | All | yes | `provider.py` holds imports, private constants and one public `Provider` subclass | +| `SQBRPYTHON015` | provider-import-boundaries | All | yes | A provider's `_helpers/` and `_classes/` are private to it; providers import only `python/utils/` from `python/` and nothing from `orchestration/` | + ### Type annotations | Code | Name | Files | Default | Checks | diff --git a/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py b/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py index e13cb556e..d86e8149c 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py @@ -3520,7 +3520,7 @@ def test_given_project_python_when_running_python_rules_then_findings_match_comp _PYTHON_ANNOTATED_ALERT, ), ), - expected_hits_and_misses=((0, 1), (0, 12), (11, 1), (0, 12), (13, 0)), + expected_hits_and_misses=((0, 1), (0, 14), (13, 1), (0, 14), (15, 0)), expected_python_cache=(False, True, True, True, True), ) ], diff --git a/website/src/content/docs/docs/concepts/python-nodes/providers.mdx b/website/src/content/docs/docs/concepts/python-nodes/providers.mdx index ecdbd805d..9492469d8 100644 --- a/website/src/content/docs/docs/concepts/python-nodes/providers.mdx +++ b/website/src/content/docs/docs/concepts/python-nodes/providers.mdx @@ -200,23 +200,30 @@ Setting values, defaults included, are never published. ## Project layout +Give each provider its own folder under `providers/`, named for the capability it provides: + ``` my-project/ providers/ - warehouse_client.py - slack_notifier.py + warehouse_client/ + provider.py # the Provider subclass + constants.py + _helpers/ # private to warehouse_client + slack_notifier/ + provider.py hooks/ sql/ record_notification.sql python/ notify.py python/ - loaders/ - load_orders.py - tasks/ - export_orders.py + orders/ + load_orders/ + node.py + main.py ``` -A project that selects the [Python Rules](/docs/concepts/rules/python-rules#laying-out-providers) -keeps each provider in its own folder, `providers//provider.py`, and each node in its -own unit folder under its domain. +Discovery also accepts `.py` files directly in `providers/`. A project that selects the +[Python Rules](/docs/concepts/rules/python-rules#laying-out-providers) reports them, along with +anything else in a provider folder besides `provider.py`, its role files, `_helpers/` and +`_classes/`. From 3378461639c79ff82097e7431360eb67960fc7dc Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sun, 15 Nov 2026 03:31:24 +0000 Subject: [PATCH 4/8] wip: resolve provider bases through imports and report tuple and subscript assignments SQBRPYTHON014 now accepts a Provider imported under an alias or reached through project base classes, reports tuple, attribute and subscript assignments in provider.py, and only parses files under providers/ or that mention providers. The providers page shows the capability-folder layout, and the violations fixture no longer hides an existing finding. --- .../python_rules/expected_findings.json | 3 +- .../providers/carrier_api/provider.py | 5 +- .../src/node_units/main/node_unit_module.rs | 15 +- .../src/node_units/models.rs | 2 +- .../python_rules/_helpers/units/providers.rs | 128 ++++++++++++++++-- .../python_rules/tests/test_provider_rules.rs | 79 +++++++++++ .../docs/concepts/python-nodes/providers.md | 6 +- .../docs/concepts/python-nodes/providers.mdx | 6 +- 8 files changed, 213 insertions(+), 31 deletions(-) diff --git a/benchmarks/duckshop/edge_cases/python_rules/expected_findings.json b/benchmarks/duckshop/edge_cases/python_rules/expected_findings.json index 64b2b5a45..94be94d1d 100644 --- a/benchmarks/duckshop/edge_cases/python_rules/expected_findings.json +++ b/benchmarks/duckshop/edge_cases/python_rules/expected_findings.json @@ -2,7 +2,7 @@ "select": ["SQBRPYTHON", "SQBRPYTHON306"], "findings": [ ["SQBRPYTHON101", "hooks/python/alert.py", 4, 11], - ["SQBRPYTHON015", "providers/carrier_api/provider.py", 3, 43], + ["SQBRPYTHON015", "providers/carrier_api/provider.py", 3, 8], ["SQBRPYTHON014", "providers/carrier_api/provider.py", 7, 5], ["SQBRPYTHON013", "providers/stock_feed.py", 1, 1], ["SQBRPYTHON613", "python/inventory/__init__.py", 1, 1], @@ -13,6 +13,7 @@ ["SQBRPYTHON614", "python/inventory/replenishment.py", 1, 1], ["SQBRPYTHON012", "python/inventory/replenishment.py", 7, 5], ["SQBRPYTHON617", "python/inventory/restock/main.py", 1, 1], + ["SQBRPYTHON306", "python/inventory/restock/main.py", 4, 1], ["SQBRPYTHON306", "python/inventory/restock/main.py", 9, 1], ["SQBRPYTHON003", "python/inventory/stock_count/node.py", 1, 1], ["SQBRPYTHON004", "python/inventory/stock_count/node.py", 5, 1], diff --git a/benchmarks/duckshop/edge_cases/python_rules/violations/providers/carrier_api/provider.py b/benchmarks/duckshop/edge_cases/python_rules/violations/providers/carrier_api/provider.py index 290a2b7fd..948e06eeb 100644 --- a/benchmarks/duckshop/edge_cases/python_rules/violations/providers/carrier_api/provider.py +++ b/benchmarks/duckshop/edge_cases/python_rules/violations/providers/carrier_api/provider.py @@ -1,13 +1,12 @@ """Carrier pickup client.""" -from python.inventory.restock.main import restock +import python.report from sqlbuild.providers import Provider def pickup_label(sku: str) -> str: """Return the pickup label of one product.""" - restock(sku) - return sku + return f"{sku} {python.report.__name__}" class CarrierApi(Provider): diff --git a/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs b/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs index 0c320bf51..564ebd3c7 100644 --- a/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs +++ b/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs @@ -38,10 +38,10 @@ pub fn node_unit_module(contents: &str, python_version: (u8, u8)) -> Option UnitStatementKind::Assignment( - assign.targets.iter().filter_map(target_name).collect(), + assign.targets.iter().flat_map(target_names).collect(), ), Stmt::AnnAssign(assign) => { - UnitStatementKind::Assignment(target_name(&assign.target).into_iter().collect()) + UnitStatementKind::Assignment(target_names(&assign.target)) } _ => UnitStatementKind::Other, }, @@ -54,10 +54,13 @@ pub fn node_unit_module(contents: &str, python_version: (u8, u8)) -> Option Option { +/// The names an assignment target binds: a plain name, or the names inside a tuple, list or starred target. +fn target_names(target: &Expr) -> Vec { match target { - Expr::Name(name) => Some(name.id.to_string()), - _ => None, + Expr::Name(name) => vec![name.id.to_string()], + Expr::Tuple(tuple) => tuple.elts.iter().flat_map(target_names).collect(), + Expr::List(list) => list.elts.iter().flat_map(target_names).collect(), + Expr::Starred(starred) => target_names(&starred.value), + _ => Vec::new(), } } diff --git a/crates/sqlbuild-pysource/src/node_units/models.rs b/crates/sqlbuild-pysource/src/node_units/models.rs index 5c6e066fa..a000e4afe 100644 --- a/crates/sqlbuild-pysource/src/node_units/models.rs +++ b/crates/sqlbuild-pysource/src/node_units/models.rs @@ -30,7 +30,7 @@ pub enum UnitStatementKind { Import, Function(UnitFunction), Class(UnitClass), - /// An assignment, annotated or not, by the names it binds. + /// An assignment, annotated or not, by the names it binds; empty when it binds an attribute or subscript. Assignment(Vec), /// Any other statement, such as a loop or a call. Other, diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs index 85a5b2fad..49569db9c 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs @@ -1,6 +1,6 @@ //! Provider capability folders: what they hold, what provider.py declares, and who imports their private code. -use std::collections::BTreeSet; +use std::collections::{BTreeMap, BTreeSet}; use rayon::iter::{IntoParallelRefIterator, ParallelIterator}; @@ -21,6 +21,9 @@ use crate::python_rules::constants::{ }; const FILE_START: UnitPosition = UnitPosition { line: 1, column: 1 }; +const OTHER_STATEMENT: &str = "a statement that is not an import, a constant or the provider class"; +/// The package Provider is imported from. +const SQLBUILD_PACKAGE: &str = "sqlbuild"; /// One parsed project file and its path parts. #[derive(Debug)] @@ -42,6 +45,10 @@ pub(crate) fn provider_faults( let mut faults: Vec = provider_layout_faults(sources); let files: Vec> = sources .par_iter() + .filter(|source| { + source.relative_path.split('/').next() == Some(PROVIDERS_ROOT) + || source.text.contains(PROVIDERS_ROOT) + }) .filter_map(|source| { Some(ParsedFile { path: source.relative_path.as_str(), @@ -54,10 +61,11 @@ pub(crate) fn provider_faults( .iter() .flat_map(|source| dotted_names(&source.relative_path)) .collect(); + let classes: ProviderClasses = provider_classes(&files, &modules); for file in &files { if matches!(file.parts.as_slice(), [root, _, file_name] if *root == PROVIDERS_ROOT && *file_name == PROVIDER_FILE) { - faults.extend(provider_module_faults(file)); + faults.extend(provider_module_faults(file, &classes)); } faults.extend(import_faults(file, &modules)); } @@ -127,12 +135,16 @@ fn provider_layout_faults(sources: &[PythonSource]) -> Vec { } /// provider.py: imports, private constants and exactly one public Provider subclass. -fn provider_module_faults(file: &ParsedFile<'_>) -> Vec { +fn provider_module_faults(file: &ParsedFile<'_>, classes: &ProviderClasses) -> Vec { + let module: String = file_module(file.path); let mut faults: Vec = Vec::new(); let mut providers: usize = 0; for statement in &file.module.statements { let (detail, position): (String, UnitPosition) = match &statement.kind { UnitStatementKind::Docstring | UnitStatementKind::Import => continue, + UnitStatementKind::Assignment(names) if names.is_empty() => { + (OTHER_STATEMENT.to_owned(), statement.position) + } UnitStatementKind::Assignment(names) => { match names.iter().find(|name| !name.starts_with(PRIVATE_PREFIX)) { Some(name) => ( @@ -143,8 +155,10 @@ fn provider_module_faults(file: &ParsedFile<'_>) -> Vec { } } UnitStatementKind::Class(class) => { - let detail: Option = class_detail(class, providers); - providers += usize::from(is_public_provider(class)); + let public_provider: bool = !class.name.starts_with(PRIVATE_PREFIX) + && classes.is_provider(&module, &class.name); + let detail: Option = class_detail(class, public_provider, providers); + providers += usize::from(public_provider); match detail { Some(detail) => (detail, class.position), None => continue, @@ -154,10 +168,7 @@ fn provider_module_faults(file: &ParsedFile<'_>) -> Vec { format!("function '{}' belongs in _helpers/", function.name), function.position, ), - UnitStatementKind::Other => ( - "a statement that is not an import, a constant or the provider class".to_owned(), - statement.position, - ), + UnitStatementKind::Other => (OTHER_STATEMENT.to_owned(), statement.position), }; faults.extend(unit_fault( PROVIDER_MODULE_SHAPE_CODE, @@ -178,20 +189,109 @@ fn provider_module_faults(file: &ParsedFile<'_>) -> Vec { } /// Why `class` does not belong in provider.py, after `earlier` public Provider subclasses. -fn class_detail(class: &UnitClass, earlier: usize) -> Option { +fn class_detail(class: &UnitClass, public_provider: bool, earlier: usize) -> Option { let name: &str = &class.name; if name.starts_with(PRIVATE_PREFIX) { return Some(format!("collaborator class '{name}' belongs in _classes/")); } - if !is_public_provider(class) { + if !public_provider { return Some(format!("class '{name}' does not subclass {PROVIDER_BASE}")); } (earlier > 0).then(|| format!("'{name}' is a second {PROVIDER_BASE} subclass")) } -/// Whether `class` is a public subclass of Provider. -fn is_public_provider(class: &UnitClass) -> bool { - !class.name.starts_with(PRIVATE_PREFIX) && class.bases.iter().any(|base| base == PROVIDER_BASE) +/// What a class's base names: Provider itself, a project class by module and name, or something unknown. +#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] +enum BaseTarget { + Provider, + Class(String, String), + Unknown, +} + +/// The bases of every class in the parsed files, by module and class name. +struct ProviderClasses { + bases: BTreeMap<(String, String), Vec>, +} + +impl ProviderClasses { + /// Whether class `name` of `module` subclasses Provider, directly, through an alias or through project classes. + fn is_provider(&self, module: &str, name: &str) -> bool { + let mut seen: BTreeSet<(String, String)> = BTreeSet::new(); + let mut pending: Vec<(String, String)> = vec![(module.to_owned(), name.to_owned())]; + while let Some(key) = pending.pop() { + if !seen.insert(key.clone()) { + continue; + } + for base in self.bases.get(&key).into_iter().flatten() { + match base { + BaseTarget::Provider => return true, + BaseTarget::Class(module, name) => pending.push((module.clone(), name.clone())), + BaseTarget::Unknown => {} + } + } + } + false + } +} + +/// Every class of `files` with what each of its bases names. +fn provider_classes(files: &[ParsedFile<'_>], modules: &BTreeSet) -> ProviderClasses { + let mut bases: BTreeMap<(String, String), Vec> = BTreeMap::new(); + for file in files { + let module: String = file_module(file.path); + for statement in &file.module.statements { + if let UnitStatementKind::Class(class) = &statement.kind { + let targets: Vec = class + .bases + .iter() + .map(|base| base_target(file, base, modules)) + .collect(); + bases.insert((module.clone(), class.name.clone()), targets); + } + } + } + ProviderClasses { bases } +} + +/// What `base` names in `file`: an import it binds, a class of the same module, or Provider by its plain name. +fn base_target(file: &ParsedFile<'_>, base: &str, modules: &BTreeSet) -> BaseTarget { + let folder: &[&str] = &file.parts[..file.parts.len() - 1]; + if let Some(import) = file + .module + .imports + .iter() + .find(|import| import.local == base) + { + let Some(name) = &import.name else { + return BaseTarget::Unknown; + }; + return match absolute_module(import, folder, modules) { + Some(source) + if source.split('.').next() == Some(SQLBUILD_PACKAGE) && name == PROVIDER_BASE => + { + BaseTarget::Provider + } + Some(source) if source.ends_with(&format!(".{name}")) => BaseTarget::Unknown, + Some(source) => BaseTarget::Class(source, name.clone()), + None => BaseTarget::Unknown, + }; + } + let local_class: bool = file.module.statements.iter().any(|statement| { + matches!(&statement.kind, UnitStatementKind::Class(class) if class.name == base) + }); + if local_class { + return BaseTarget::Class(file_module(file.path), base.to_owned()); + } + if base == PROVIDER_BASE { + BaseTarget::Provider + } else { + BaseTarget::Unknown + } +} + +/// The dotted module of a project file, such as `providers.storefront_api.provider`. +fn file_module(path: &str) -> String { + dotted_names(path).pop().unwrap_or_default() } /// Imports of another provider's private code, and a provider's imports of python/ outside utils/ or of orchestration/. diff --git a/crates/sqlbuild-rules/src/python_rules/tests/test_provider_rules.rs b/crates/sqlbuild-rules/src/python_rules/tests/test_provider_rules.rs index d4bcb2659..65952fdd5 100644 --- a/crates/sqlbuild-rules/src/python_rules/tests/test_provider_rules.rs +++ b/crates/sqlbuild-rules/src/python_rules/tests/test_provider_rules.rs @@ -132,6 +132,85 @@ class CarrierApi(Provider): ), ], }, + PythonRunTestCase { + description: "provider.py: Provider through an alias or through _classes/ bases that reach it by relative imports", + files: &[ + ( + "providers/storefront_api/provider.py", + r#"from ._classes.paged import PagedApi + + +class StorefrontApi(PagedApi): + """Client.""" +"#, + ), + ( + "providers/storefront_api/_classes/paged.py", + "from .base import BaseApi\n\n\nclass PagedApi(BaseApi):\n pass\n", + ), + ( + "providers/storefront_api/_classes/base.py", + "from sqlbuild.providers import Provider\n\n\nclass BaseApi(Provider):\n pass\n", + ), + ( + "providers/carrier_api/provider.py", + r#"from sqlbuild.providers import Provider as SqbProvider + + +class CarrierApi(SqbProvider): + """Client.""" +"#, + ), + ], + config: json!({ "select": ["SQBRPYTHON014"] }), + expected_faults: vec![], + }, + PythonRunTestCase { + description: "provider.py: tuple and subscript assignments, and a Provider imported from outside sqlbuild", + files: &[ + ( + "providers/storefront_api/provider.py", + r#"import os + +from sqlbuild.providers import Provider + +_HOST, _PORT = "h", 1 +HOST, _TIMEOUT = "h", 3 +os.environ["TZ"] = "UTC" + + +class StorefrontApi(Provider): + """Client.""" +"#, + ), + ( + "providers/carrier_api/provider.py", + r#"from vendor.providers import Provider + + +class CarrierApi(Provider): + """Client.""" +"#, + ), + ], + config: json!({ "select": ["SQBRPYTHON014"] }), + expected_faults: vec![ + ("SQBRPYTHON014", "providers/carrier_api/provider.py", 1, 1), + ("SQBRPYTHON014", "providers/carrier_api/provider.py", 4, 7), + ( + "SQBRPYTHON014", + "providers/storefront_api/provider.py", + 6, + 1, + ), + ( + "SQBRPYTHON014", + "providers/storefront_api/provider.py", + 7, + 1, + ), + ], + }, PythonRunTestCase { description: "imports: another provider's private code from anywhere, and python/ outside utils/ or orchestration/ from a provider", files: &[ diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/python-nodes/providers.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/python-nodes/providers.md index f131ceb30..c9be71b78 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/python-nodes/providers.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/python-nodes/providers.md @@ -24,10 +24,10 @@ Providers are shared runtime services that SQLBuild discovers, configures, and i ## Defining a provider -Create a Python file under `providers/` in your project. A provider is a class that subclasses `Provider` from `sqlbuild.providers`: +Create a `provider.py` in a capability folder under `providers/` (see [Project layout](#project-layout)). A provider is a class that subclasses `Provider` from `sqlbuild.providers`: ```python -# providers/warehouse_client.py +# providers/warehouse_client/provider.py from sqlbuild.providers import Provider class WarehouseClient(Provider): @@ -83,7 +83,7 @@ You can also type-annotate the parameter for IDE support and compile-time valida ```python from sqlbuild.tasks import task -from providers.warehouse_client import WarehouseClient +from providers.warehouse_client.provider import WarehouseClient @task def export_orders(ctx, warehouse_client: WarehouseClient): diff --git a/website/src/content/docs/docs/concepts/python-nodes/providers.mdx b/website/src/content/docs/docs/concepts/python-nodes/providers.mdx index 9492469d8..d8f140c41 100644 --- a/website/src/content/docs/docs/concepts/python-nodes/providers.mdx +++ b/website/src/content/docs/docs/concepts/python-nodes/providers.mdx @@ -7,10 +7,10 @@ Providers are shared runtime services that SQLBuild discovers, configures, and i ## Defining a provider -Create a Python file under `providers/` in your project. A provider is a class that subclasses `Provider` from `sqlbuild.providers`: +Create a `provider.py` in a capability folder under `providers/` (see [Project layout](#project-layout)). A provider is a class that subclasses `Provider` from `sqlbuild.providers`: ```python -# providers/warehouse_client.py +# providers/warehouse_client/provider.py from sqlbuild.providers import Provider class WarehouseClient(Provider): @@ -66,7 +66,7 @@ You can also type-annotate the parameter for IDE support and compile-time valida ```python from sqlbuild.tasks import task -from providers.warehouse_client import WarehouseClient +from providers.warehouse_client.provider import WarehouseClient @task def export_orders(ctx, warehouse_client: WarehouseClient): From 8a046dd20f7887d5e61d3cfd01af569054e1d873 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sun, 15 Nov 2026 03:48:17 +0000 Subject: [PATCH 5/8] wip: decide SQBRPYTHON014 Provider bases as provider, not a provider, or unknown Keep dotted base names so module attribute bases resolve, report a class as not a Provider only when every base chain resolves to project classes or builtins, and stay silent when a chain leaves the parsed sources. Any assignment with an attribute or subscript target is a statement that does not belong in provider.py. --- .../src/node_units/main/node_unit_module.rs | 55 +++++++-- .../src/node_units/models.rs | 5 +- .../python_rules/_helpers/units/providers.rs | 116 +++++++++++++----- .../python_rules/tests/test_provider_rules.rs | 71 +++++++++-- 4 files changed, 196 insertions(+), 51 deletions(-) diff --git a/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs b/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs index 564ebd3c7..720e0795f 100644 --- a/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs +++ b/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs @@ -5,7 +5,7 @@ use ruff_text_size::Ranged; use crate::cpython_tree::main::parse_cpython_module::parse_cpython_module; use crate::cpython_tree::models::ParsedSource; -use crate::node_units::_helpers::functions::{last_name, leading_docstring, unit_function}; +use crate::node_units::_helpers::functions::{leading_docstring, unit_function}; use crate::node_units::_helpers::imports::{is_type_checking_imports, module_imports}; use crate::node_units::_helpers::positions::{UnitText, unit_position}; use crate::node_units::models::{UnitClass, UnitModule, UnitStatement, UnitStatementKind}; @@ -35,13 +35,23 @@ pub fn node_unit_module(contents: &str, python_version: (u8, u8)) -> Option UnitStatementKind::Class(UnitClass { name: class.name.to_string(), position: unit_position(&source, class.name.range().start()), - bases: class.bases().iter().filter_map(last_name).collect(), + bases: class + .bases() + .iter() + .map(|base| dotted_name(base).unwrap_or_default()) + .collect(), }), Stmt::Assign(assign) => UnitStatementKind::Assignment( - assign.targets.iter().flat_map(target_names).collect(), + assign + .targets + .iter() + .map(target_names) + .collect::>>>() + .map(|names| names.concat()) + .unwrap_or_default(), ), Stmt::AnnAssign(assign) => { - UnitStatementKind::Assignment(target_names(&assign.target)) + UnitStatementKind::Assignment(target_names(&assign.target).unwrap_or_default()) } _ => UnitStatementKind::Other, }, @@ -54,13 +64,38 @@ pub fn node_unit_module(contents: &str, python_version: (u8, u8)) -> Option Vec { +/// The names an assignment target binds, through tuples, lists and starred targets; `None` when any part +/// of it is an attribute or subscript. +fn target_names(target: &Expr) -> Option> { match target { - Expr::Name(name) => vec![name.id.to_string()], - Expr::Tuple(tuple) => tuple.elts.iter().flat_map(target_names).collect(), - Expr::List(list) => list.elts.iter().flat_map(target_names).collect(), + Expr::Name(name) => Some(vec![name.id.to_string()]), + Expr::Tuple(tuple) => elements_names(&tuple.elts), + Expr::List(list) => elements_names(&list.elts), Expr::Starred(starred) => target_names(&starred.value), - _ => Vec::new(), + _ => None, + } +} + +/// The names the elements of a tuple or list target bind; `None` when any of them binds no plain name. +fn elements_names(elements: &[Expr]) -> Option> { + elements + .iter() + .map(target_names) + .collect::>>>() + .map(|names| names.concat()) +} + +/// A name or attribute chain as dotted text, such as `paged.PagedApi`, seen through a subscript such as +/// `Generic[T]`; `None` for any other expression. +fn dotted_name(expression: &Expr) -> Option { + match expression { + Expr::Name(name) => Some(name.id.to_string()), + Expr::Attribute(attribute) => Some(format!( + "{}.{}", + dotted_name(&attribute.value)?, + attribute.attr + )), + Expr::Subscript(subscript) => dotted_name(&subscript.value), + _ => None, } } diff --git a/crates/sqlbuild-pysource/src/node_units/models.rs b/crates/sqlbuild-pysource/src/node_units/models.rs index a000e4afe..e1587cd6d 100644 --- a/crates/sqlbuild-pysource/src/node_units/models.rs +++ b/crates/sqlbuild-pysource/src/node_units/models.rs @@ -30,18 +30,19 @@ pub enum UnitStatementKind { Import, Function(UnitFunction), Class(UnitClass), - /// An assignment, annotated or not, by the names it binds; empty when it binds an attribute or subscript. + /// An assignment, annotated or not, by the names it binds; empty when any target is an attribute or subscript. Assignment(Vec), /// Any other statement, such as a loop or a call. Other, } -/// A class and the last name of each of its bases. +/// A class and its bases. #[derive(Clone, Debug, PartialEq, Eq)] pub struct UnitClass { pub name: String, /// Where the class's name is written. pub position: UnitPosition, + /// Each base as dotted text, such as `Provider` or `paged.PagedApi`; empty when it is another expression. pub bases: Vec, } diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs index 49569db9c..65f6f485a 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs @@ -139,6 +139,7 @@ fn provider_module_faults(file: &ParsedFile<'_>, classes: &ProviderClasses) -> V let module: String = file_module(file.path); let mut faults: Vec = Vec::new(); let mut providers: usize = 0; + let mut undecided: usize = 0; for statement in &file.module.statements { let (detail, position): (String, UnitPosition) = match &statement.kind { UnitStatementKind::Docstring | UnitStatementKind::Import => continue, @@ -155,10 +156,12 @@ fn provider_module_faults(file: &ParsedFile<'_>, classes: &ProviderClasses) -> V } } UnitStatementKind::Class(class) => { - let public_provider: bool = !class.name.starts_with(PRIVATE_PREFIX) - && classes.is_provider(&module, &class.name); - let detail: Option = class_detail(class, public_provider, providers); - providers += usize::from(public_provider); + let verdict: Verdict = classes.verdict(&module, &class.name); + let detail: Option = class_detail(class, verdict, providers); + if !class.name.starts_with(PRIVATE_PREFIX) { + providers += usize::from(verdict == Verdict::Provider); + undecided += usize::from(verdict == Verdict::Unknown); + } match detail { Some(detail) => (detail, class.position), None => continue, @@ -177,7 +180,7 @@ fn provider_module_faults(file: &ParsedFile<'_>, classes: &ProviderClasses) -> V &detail, )); } - if providers == 0 { + if providers == 0 && undecided == 0 { faults.extend(unit_fault( PROVIDER_MODULE_SHAPE_CODE, file.path, @@ -189,22 +192,36 @@ fn provider_module_faults(file: &ParsedFile<'_>, classes: &ProviderClasses) -> V } /// Why `class` does not belong in provider.py, after `earlier` public Provider subclasses. -fn class_detail(class: &UnitClass, public_provider: bool, earlier: usize) -> Option { +fn class_detail(class: &UnitClass, verdict: Verdict, earlier: usize) -> Option { let name: &str = &class.name; if name.starts_with(PRIVATE_PREFIX) { return Some(format!("collaborator class '{name}' belongs in _classes/")); } - if !public_provider { - return Some(format!("class '{name}' does not subclass {PROVIDER_BASE}")); + match verdict { + Verdict::NotProvider => Some(format!("class '{name}' does not subclass {PROVIDER_BASE}")), + Verdict::Provider => { + (earlier > 0).then(|| format!("'{name}' is a second {PROVIDER_BASE} subclass")) + } + Verdict::Unknown => None, } - (earlier > 0).then(|| format!("'{name}' is a second {PROVIDER_BASE} subclass")) } -/// What a class's base names: Provider itself, a project class by module and name, or something unknown. +/// Whether a class subclasses Provider, as far as the parsed project sources show. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum Verdict { + Provider, + NotProvider, + /// A base leads outside what was parsed, such as another package, so the rule stays silent. + Unknown, +} + +/// What a class's base names: Provider itself, a project class by module and name, a builtin, or +/// something the parsed sources cannot resolve. #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] enum BaseTarget { Provider, Class(String, String), + Builtin, Unknown, } @@ -214,23 +231,34 @@ struct ProviderClasses { } impl ProviderClasses { - /// Whether class `name` of `module` subclasses Provider, directly, through an alias or through project classes. - fn is_provider(&self, module: &str, name: &str) -> bool { + /// Whether class `name` of `module` subclasses Provider, directly, through an alias or through project + /// classes; `Unknown` when a base chain leaves the parsed classes before reaching Provider. + fn verdict(&self, module: &str, name: &str) -> Verdict { let mut seen: BTreeSet<(String, String)> = BTreeSet::new(); let mut pending: Vec<(String, String)> = vec![(module.to_owned(), name.to_owned())]; + let mut unknown: bool = false; while let Some(key) = pending.pop() { if !seen.insert(key.clone()) { continue; } - for base in self.bases.get(&key).into_iter().flatten() { + let Some(bases) = self.bases.get(&key) else { + unknown = true; + continue; + }; + for base in bases { match base { - BaseTarget::Provider => return true, + BaseTarget::Provider => return Verdict::Provider, BaseTarget::Class(module, name) => pending.push((module.clone(), name.clone())), - BaseTarget::Unknown => {} + BaseTarget::Builtin => {} + BaseTarget::Unknown => unknown = true, } } } - false + if unknown { + Verdict::Unknown + } else { + Verdict::NotProvider + } } } @@ -253,39 +281,65 @@ fn provider_classes(files: &[ParsedFile<'_>], modules: &BTreeSet) -> Pro ProviderClasses { bases } } -/// What `base` names in `file`: an import it binds, a class of the same module, or Provider by its plain name. +/// What `base` names in `file`: Provider, a class of a project module reached through an import or the +/// same module, a builtin, or something unknown such as another package's class. fn base_target(file: &ParsedFile<'_>, base: &str, modules: &BTreeSet) -> BaseTarget { + let (first, attributes): (&str, Option<&str>) = match base.split_once('.') { + Some((first, attributes)) => (first, Some(attributes)), + None => (base, None), + }; + if first.is_empty() { + return BaseTarget::Unknown; + } let folder: &[&str] = &file.parts[..file.parts.len() - 1]; if let Some(import) = file .module .imports .iter() - .find(|import| import.local == base) + .find(|import| import.local == first) { - let Some(name) = &import.name else { + let Some(source) = absolute_module(import, folder, modules) else { return BaseTarget::Unknown; }; - return match absolute_module(import, folder, modules) { - Some(source) - if source.split('.').next() == Some(SQLBUILD_PACKAGE) && name == PROVIDER_BASE => - { - BaseTarget::Provider + let (module, name): (String, &str) = match (&import.name, attributes) { + (Some(name), None) if source.ends_with(&format!(".{name}")) => { + return BaseTarget::Unknown; + } + (Some(name), None) => (source, name.as_str()), + (bound, Some(attributes)) => { + let bound_module: String = match bound { + Some(name) if source.ends_with(&format!(".{name}")) => source, + Some(_) => return BaseTarget::Unknown, + None if import.module.split('.').next() == Some(import.local.as_str()) => { + import.local.clone() + } + None => source, + }; + match attributes.rsplit_once('.') { + Some((path, name)) => (format!("{bound_module}.{path}"), name), + None => (bound_module, attributes), + } } - Some(source) if source.ends_with(&format!(".{name}")) => BaseTarget::Unknown, - Some(source) => BaseTarget::Class(source, name.clone()), - None => BaseTarget::Unknown, + (None, None) => return BaseTarget::Unknown, }; + return if module.split('.').next() == Some(SQLBUILD_PACKAGE) && name == PROVIDER_BASE { + BaseTarget::Provider + } else { + BaseTarget::Class(module, name.to_owned()) + }; + } + if attributes.is_some() || file.module.imports.iter().any(|import| import.local == "*") { + return BaseTarget::Unknown; } let local_class: bool = file.module.statements.iter().any(|statement| { matches!(&statement.kind, UnitStatementKind::Class(class) if class.name == base) }); if local_class { - return BaseTarget::Class(file_module(file.path), base.to_owned()); - } - if base == PROVIDER_BASE { + BaseTarget::Class(file_module(file.path), base.to_owned()) + } else if base == PROVIDER_BASE { BaseTarget::Provider } else { - BaseTarget::Unknown + BaseTarget::Builtin } } diff --git a/crates/sqlbuild-rules/src/python_rules/tests/test_provider_rules.rs b/crates/sqlbuild-rules/src/python_rules/tests/test_provider_rules.rs index 65952fdd5..1dbe18cb4 100644 --- a/crates/sqlbuild-rules/src/python_rules/tests/test_provider_rules.rs +++ b/crates/sqlbuild-rules/src/python_rules/tests/test_provider_rules.rs @@ -133,7 +133,7 @@ class CarrierApi(Provider): ], }, PythonRunTestCase { - description: "provider.py: Provider through an alias or through _classes/ bases that reach it by relative imports", + description: "provider.py: Provider through an alias, a module attribute or _classes/ bases reached by relative imports; bases outside the parsed sources stay silent", files: &[ ( "providers/storefront_api/provider.py", @@ -161,12 +161,48 @@ class CarrierApi(SqbProvider): """Client.""" "#, ), + ( + "providers/billing_api/provider.py", + "from ._classes import paged\n\n\nclass BillingApi(paged.PagedApi):\n \"\"\"Client.\"\"\"\n", + ), + ( + "providers/billing_api/_classes/paged.py", + "from sqlbuild.providers import Provider\n\n\nclass PagedApi(Provider):\n pass\n", + ), + ( + "providers/ledger_api/provider.py", + "from ._classes import LedgerBase\n\n\nclass LedgerApi(LedgerBase):\n \"\"\"Client.\"\"\"\n", + ), + ( + "providers/ledger_api/_classes/__init__.py", + "from .base import LedgerBase\n", + ), + ( + "providers/ledger_api/_classes/base.py", + "from sqlbuild.providers import Provider\n\n\nclass LedgerBase(Provider):\n pass\n", + ), + ( + "providers/catalog_api/provider.py", + "from python.utils.clients import PagedClient\n\n\nclass CatalogApi(PagedClient):\n \"\"\"Client.\"\"\"\n", + ), + ( + "python/utils/clients.py", + "from python.utils.base import BaseClient\n\n\nclass PagedClient(BaseClient):\n pass\n", + ), + ( + "python/utils/base.py", + "from sqlbuild.providers import Provider\n\n\nclass BaseClient(Provider):\n pass\n", + ), + ( + "providers/partner_api/provider.py", + "from vendor.clients import VendorProvider\n\n\nclass PartnerApi(VendorProvider):\n \"\"\"Client.\"\"\"\n", + ), ], config: json!({ "select": ["SQBRPYTHON014"] }), expected_faults: vec![], }, PythonRunTestCase { - description: "provider.py: tuple and subscript assignments, and a Provider imported from outside sqlbuild", + description: "provider.py: tuple, subscript, mixed and chained assignments, a builtin base and a module attribute naming a plain class", files: &[ ( "providers/storefront_api/provider.py", @@ -177,6 +213,8 @@ from sqlbuild.providers import Provider _HOST, _PORT = "h", 1 HOST, _TIMEOUT = "h", 3 os.environ["TZ"] = "UTC" +_ZONE, os.environ["TZ"] = "h", "UTC" +_TZ = os.environ["TZ"] = "UTC" class StorefrontApi(Provider): @@ -185,18 +223,23 @@ class StorefrontApi(Provider): ), ( "providers/carrier_api/provider.py", - r#"from vendor.providers import Provider - - -class CarrierApi(Provider): - """Client.""" -"#, + "from ._classes import shapes\n\n\nclass CarrierApi(shapes.Parcel):\n \"\"\"Client.\"\"\"\n", + ), + ( + "providers/carrier_api/_classes/shapes.py", + "class Parcel:\n pass\n", + ), + ( + "providers/returns_api/provider.py", + "class ReturnsApi(Exception):\n \"\"\"Client.\"\"\"\n", ), ], config: json!({ "select": ["SQBRPYTHON014"] }), expected_faults: vec![ ("SQBRPYTHON014", "providers/carrier_api/provider.py", 1, 1), ("SQBRPYTHON014", "providers/carrier_api/provider.py", 4, 7), + ("SQBRPYTHON014", "providers/returns_api/provider.py", 1, 1), + ("SQBRPYTHON014", "providers/returns_api/provider.py", 1, 7), ( "SQBRPYTHON014", "providers/storefront_api/provider.py", @@ -209,6 +252,18 @@ class CarrierApi(Provider): 7, 1, ), + ( + "SQBRPYTHON014", + "providers/storefront_api/provider.py", + 8, + 1, + ), + ( + "SQBRPYTHON014", + "providers/storefront_api/provider.py", + 9, + 1, + ), ], }, PythonRunTestCase { From 3e89896897e70595b6bda9f6c61a171ae0fdb212 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sun, 15 Nov 2026 03:58:54 +0000 Subject: [PATCH 6/8] wip: document when SQBRPYTHON014 stays silent about a class's bases --- .../sqlbuild/references/docs/concepts/rules/python-rules.md | 4 ++++ website/src/content/docs/docs/concepts/rules/python-rules.mdx | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/python-rules.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/python-rules.md index eb807414e..78938aa8f 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/python-rules.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/python-rules.md @@ -185,6 +185,10 @@ A capability folder may also hold `types.py`, `constants.py`, `exceptions.py`, ` | `SQBRPYTHON014` | `provider.py` holding a function, a public constant, another class or a second provider | Keep one public `Provider` subclass and private constants; move the rest to `_helpers/`, `_classes/` or `constants.py` | | `SQBRPYTHON015` | An import of a provider's `_helpers/` or `_classes/` from outside its folder, or a provider importing `python/` outside `utils/` or anything in `orchestration/` | Import the provider through `provider.py` or its role files; share code through `python/utils/` | +`SQBRPYTHON014` follows a class's bases through the project's modules, so a provider may subclass +`Provider` through an alias or a base class in `_classes/`. A class whose base comes from another +package is never reported as not being a provider. + ## Main modules and entry modules Some Rules apply only to a domain's main modules: files under a `main/` folder, and a unit's diff --git a/website/src/content/docs/docs/concepts/rules/python-rules.mdx b/website/src/content/docs/docs/concepts/rules/python-rules.mdx index 6e19045de..5e2c3263b 100644 --- a/website/src/content/docs/docs/concepts/rules/python-rules.mdx +++ b/website/src/content/docs/docs/concepts/rules/python-rules.mdx @@ -170,6 +170,10 @@ A capability folder may also hold `types.py`, `constants.py`, `exceptions.py`, ` | `SQBRPYTHON014` | `provider.py` holding a function, a public constant, another class or a second provider | Keep one public `Provider` subclass and private constants; move the rest to `_helpers/`, `_classes/` or `constants.py` | | `SQBRPYTHON015` | An import of a provider's `_helpers/` or `_classes/` from outside its folder, or a provider importing `python/` outside `utils/` or anything in `orchestration/` | Import the provider through `provider.py` or its role files; share code through `python/utils/` | +`SQBRPYTHON014` follows a class's bases through the project's modules, so a provider may subclass +`Provider` through an alias or a base class in `_classes/`. A class whose base comes from another +package is never reported as not being a provider. + ## Main modules and entry modules Some Rules apply only to a domain's main modules: files under a `main/` folder, and a unit's From 1fefcab1c50aac2827f1f275256eeab1f0e4595e Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sun, 15 Nov 2026 04:03:54 +0000 Subject: [PATCH 7/8] wip: sort provider fixture imports --- .../python_rules/conforming/providers/carrier_api/provider.py | 1 + .../duckshop/edge_cases/python_rules/expected_findings.json | 2 +- .../python_rules/violations/providers/carrier_api/provider.py | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/benchmarks/duckshop/edge_cases/python_rules/conforming/providers/carrier_api/provider.py b/benchmarks/duckshop/edge_cases/python_rules/conforming/providers/carrier_api/provider.py index ce2994ac3..86672d2b3 100644 --- a/benchmarks/duckshop/edge_cases/python_rules/conforming/providers/carrier_api/provider.py +++ b/benchmarks/duckshop/edge_cases/python_rules/conforming/providers/carrier_api/provider.py @@ -1,6 +1,7 @@ """Carrier pickup client.""" from providers.carrier_api.constants import MINIMUM_CHARGE_CENTS + from sqlbuild.providers import Provider _CENTS_PER_UNIT: int = 100 diff --git a/benchmarks/duckshop/edge_cases/python_rules/expected_findings.json b/benchmarks/duckshop/edge_cases/python_rules/expected_findings.json index 94be94d1d..f7ae9dfa5 100644 --- a/benchmarks/duckshop/edge_cases/python_rules/expected_findings.json +++ b/benchmarks/duckshop/edge_cases/python_rules/expected_findings.json @@ -3,7 +3,7 @@ "findings": [ ["SQBRPYTHON101", "hooks/python/alert.py", 4, 11], ["SQBRPYTHON015", "providers/carrier_api/provider.py", 3, 8], - ["SQBRPYTHON014", "providers/carrier_api/provider.py", 7, 5], + ["SQBRPYTHON014", "providers/carrier_api/provider.py", 8, 5], ["SQBRPYTHON013", "providers/stock_feed.py", 1, 1], ["SQBRPYTHON613", "python/inventory/__init__.py", 1, 1], ["SQBRPYTHON002", "python/inventory/common/__init__.py", 1, 1], diff --git a/benchmarks/duckshop/edge_cases/python_rules/violations/providers/carrier_api/provider.py b/benchmarks/duckshop/edge_cases/python_rules/violations/providers/carrier_api/provider.py index 948e06eeb..4a3dedd1d 100644 --- a/benchmarks/duckshop/edge_cases/python_rules/violations/providers/carrier_api/provider.py +++ b/benchmarks/duckshop/edge_cases/python_rules/violations/providers/carrier_api/provider.py @@ -1,6 +1,7 @@ """Carrier pickup client.""" import python.report + from sqlbuild.providers import Provider From ff308e861e7410c0b870110b1edee5a4165d015e Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sun, 15 Nov 2026 04:11:43 +0000 Subject: [PATCH 8/8] wip: move assignment and base name readers into a pysource helper --- .../src/node_units/_helpers/mod.rs | 1 + .../src/node_units/_helpers/targets.rs | 49 +++++++++++++++ .../src/node_units/main/node_unit_module.rs | 61 +++---------------- .../python_rules/_helpers/units/providers.rs | 19 +++--- .../src/python_rules/constants.rs | 2 + 5 files changed, 71 insertions(+), 61 deletions(-) create mode 100644 crates/sqlbuild-pysource/src/node_units/_helpers/targets.rs diff --git a/crates/sqlbuild-pysource/src/node_units/_helpers/mod.rs b/crates/sqlbuild-pysource/src/node_units/_helpers/mod.rs index 0d018f6f9..da1b2da49 100644 --- a/crates/sqlbuild-pysource/src/node_units/_helpers/mod.rs +++ b/crates/sqlbuild-pysource/src/node_units/_helpers/mod.rs @@ -1,3 +1,4 @@ pub(crate) mod functions; pub(crate) mod imports; pub(crate) mod positions; +pub(crate) mod targets; diff --git a/crates/sqlbuild-pysource/src/node_units/_helpers/targets.rs b/crates/sqlbuild-pysource/src/node_units/_helpers/targets.rs new file mode 100644 index 000000000..b008ce826 --- /dev/null +++ b/crates/sqlbuild-pysource/src/node_units/_helpers/targets.rs @@ -0,0 +1,49 @@ +//! The names assignments bind and the dotted names of class bases. + +use ruff_python_ast::Expr; + +/// The names an assignment's targets bind; empty when any target is an attribute or subscript. +pub(crate) fn assignment_names(targets: &[Expr]) -> Vec { + elements_names(targets).unwrap_or_default() +} + +/// Each class base as dotted text, such as `paged.PagedApi`; empty for any other expression. +pub(crate) fn class_bases(bases: &[Expr]) -> Vec { + bases.iter().map(base_name).collect() +} + +/// One class base as dotted text, or empty. +fn base_name(base: &Expr) -> String { + dotted_name(base).unwrap_or_default() +} + +/// The names one target binds through tuples, lists and starred targets; `None` for an attribute or subscript. +fn target_names(target: &Expr) -> Option> { + match target { + Expr::Name(name) => Some(vec![name.id.to_string()]), + Expr::Tuple(tuple) => elements_names(&tuple.elts), + Expr::List(list) => elements_names(&list.elts), + Expr::Starred(starred) => target_names(&starred.value), + _ => None, + } +} + +/// The names several targets bind; `None` when any of them binds no plain name. +fn elements_names(elements: &[Expr]) -> Option> { + let names: Vec> = elements.iter().map(target_names).collect::>()?; + Some(names.concat()) +} + +/// A name or attribute chain as dotted text, seen through a subscript such as `Generic[T]`. +fn dotted_name(expression: &Expr) -> Option { + match expression { + Expr::Name(name) => Some(name.id.to_string()), + Expr::Attribute(attribute) => Some(format!( + "{}.{}", + dotted_name(&attribute.value)?, + attribute.attr + )), + Expr::Subscript(subscript) => dotted_name(&subscript.value), + _ => None, + } +} diff --git a/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs b/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs index 720e0795f..3197611d3 100644 --- a/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs +++ b/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs @@ -1,6 +1,6 @@ //! Read a module under `python/` for the node unit rules, without importing it. -use ruff_python_ast::{Expr, Stmt}; +use ruff_python_ast::Stmt; use ruff_text_size::Ranged; use crate::cpython_tree::main::parse_cpython_module::parse_cpython_module; @@ -8,6 +8,7 @@ use crate::cpython_tree::models::ParsedSource; use crate::node_units::_helpers::functions::{leading_docstring, unit_function}; use crate::node_units::_helpers::imports::{is_type_checking_imports, module_imports}; use crate::node_units::_helpers::positions::{UnitText, unit_position}; +use crate::node_units::_helpers::targets::{assignment_names, class_bases}; use crate::node_units::models::{UnitClass, UnitModule, UnitStatement, UnitStatementKind}; /// The top-level statements and imports of `contents`, or `None` when it does not parse. @@ -35,24 +36,14 @@ pub fn node_unit_module(contents: &str, python_version: (u8, u8)) -> Option UnitStatementKind::Class(UnitClass { name: class.name.to_string(), position: unit_position(&source, class.name.range().start()), - bases: class - .bases() - .iter() - .map(|base| dotted_name(base).unwrap_or_default()) - .collect(), + bases: class_bases(class.bases()), }), - Stmt::Assign(assign) => UnitStatementKind::Assignment( - assign - .targets - .iter() - .map(target_names) - .collect::>>>() - .map(|names| names.concat()) - .unwrap_or_default(), - ), - Stmt::AnnAssign(assign) => { - UnitStatementKind::Assignment(target_names(&assign.target).unwrap_or_default()) + Stmt::Assign(assign) => { + UnitStatementKind::Assignment(assignment_names(&assign.targets)) } + Stmt::AnnAssign(assign) => UnitStatementKind::Assignment(assignment_names( + std::slice::from_ref(&assign.target), + )), _ => UnitStatementKind::Other, }, position: unit_position(&source, statement.range().start()), @@ -63,39 +54,3 @@ pub fn node_unit_module(contents: &str, python_version: (u8, u8)) -> Option Option> { - match target { - Expr::Name(name) => Some(vec![name.id.to_string()]), - Expr::Tuple(tuple) => elements_names(&tuple.elts), - Expr::List(list) => elements_names(&list.elts), - Expr::Starred(starred) => target_names(&starred.value), - _ => None, - } -} - -/// The names the elements of a tuple or list target bind; `None` when any of them binds no plain name. -fn elements_names(elements: &[Expr]) -> Option> { - elements - .iter() - .map(target_names) - .collect::>>>() - .map(|names| names.concat()) -} - -/// A name or attribute chain as dotted text, such as `paged.PagedApi`, seen through a subscript such as -/// `Generic[T]`; `None` for any other expression. -fn dotted_name(expression: &Expr) -> Option { - match expression { - Expr::Name(name) => Some(name.id.to_string()), - Expr::Attribute(attribute) => Some(format!( - "{}.{}", - dotted_name(&attribute.value)?, - attribute.attr - )), - Expr::Subscript(subscript) => dotted_name(&subscript.value), - _ => None, - } -} diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs index 65f6f485a..971166a87 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs @@ -17,7 +17,7 @@ use crate::python_rules::constants::{ INIT_FILE, ORCHESTRATION_ROOT, PACKAGE_ROOT, PRIVATE_PREFIX, PROVIDER_BASE, PROVIDER_FILE, PROVIDER_IMPORT_BOUNDARIES_CODE, PROVIDER_LAYOUT_CODE, PROVIDER_MODULE_SHAPE_CODE, PROVIDER_PRIVATE_FOLDERS, PROVIDER_ROLE_FILES, PROVIDER_RULE_CODES, PROVIDERS_ROOT, - SHARED_UTILITIES_PACKAGE, + SHARED_UTILITIES_PACKAGE, STAR_IMPORT, }; const FILE_START: UnitPosition = UnitPosition { line: 1, column: 1 }; @@ -215,8 +215,7 @@ enum Verdict { Unknown, } -/// What a class's base names: Provider itself, a project class by module and name, a builtin, or -/// something the parsed sources cannot resolve. +/// What a class's base names: Provider, a project class, a builtin, or something unresolved. #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] enum BaseTarget { Provider, @@ -231,8 +230,7 @@ struct ProviderClasses { } impl ProviderClasses { - /// Whether class `name` of `module` subclasses Provider, directly, through an alias or through project - /// classes; `Unknown` when a base chain leaves the parsed classes before reaching Provider. + /// Whether class `name` of `module` reaches Provider; `Unknown` when a base chain leaves the parsed classes. fn verdict(&self, module: &str, name: &str) -> Verdict { let mut seen: BTreeSet<(String, String)> = BTreeSet::new(); let mut pending: Vec<(String, String)> = vec![(module.to_owned(), name.to_owned())]; @@ -281,8 +279,7 @@ fn provider_classes(files: &[ParsedFile<'_>], modules: &BTreeSet) -> Pro ProviderClasses { bases } } -/// What `base` names in `file`: Provider, a class of a project module reached through an import or the -/// same module, a builtin, or something unknown such as another package's class. +/// What `base` names in `file`, resolved through its imports and its own classes. fn base_target(file: &ParsedFile<'_>, base: &str, modules: &BTreeSet) -> BaseTarget { let (first, attributes): (&str, Option<&str>) = match base.split_once('.') { Some((first, attributes)) => (first, Some(attributes)), @@ -328,7 +325,13 @@ fn base_target(file: &ParsedFile<'_>, base: &str, modules: &BTreeSet) -> BaseTarget::Class(module, name.to_owned()) }; } - if attributes.is_some() || file.module.imports.iter().any(|import| import.local == "*") { + if attributes.is_some() + || file + .module + .imports + .iter() + .any(|import| import.local == STAR_IMPORT) + { return BaseTarget::Unknown; } let local_class: bool = file.module.statements.iter().any(|statement| { diff --git a/crates/sqlbuild-rules/src/python_rules/constants.rs b/crates/sqlbuild-rules/src/python_rules/constants.rs index a7cfe9245..77649efd2 100644 --- a/crates/sqlbuild-rules/src/python_rules/constants.rs +++ b/crates/sqlbuild-rules/src/python_rules/constants.rs @@ -1099,6 +1099,8 @@ pub(crate) const PROVIDER_BASE: &str = "Provider"; pub(crate) const ORCHESTRATION_ROOT: &str = "orchestration"; /// The prefix of a private name. pub(crate) const PRIVATE_PREFIX: &str = "_"; +/// The name a `from module import *` binds. +pub(crate) const STAR_IMPORT: &str = "*"; /// The file that declares a unit's node and makes its folder a unit. pub(crate) const UNIT_NODE_FILE: &str = "node.py"; /// The file holding a unit's checks.