From a2c840f50114ff6b0b18d77770a6d5a49a3d6a34 Mon Sep 17 00:00:00 2001 From: Brian Thorne Date: Fri, 22 May 2026 15:52:12 +1200 Subject: [PATCH] Require Ruff when formatting Python codegen Fail Python codegen when ruff format is requested but Ruff is missing or fails, and make --format=false a working escape hatch. Document Ruff as a formatter dependency and pin the CI Ruff version used by Rust tests. --- .github/workflows/ci.yaml | 1 + CHANGELOG.md | 2 + README.md | 2 +- reflectapi-cli/src/main.rs | 9 +- reflectapi-cli/tests/output_paths.rs | 118 ++++++++++++++++ ...sts__basic__reflectapi_struct_tuple-5.snap | 18 ++- reflectapi/src/codegen/python.rs | 127 +++++++++++------- 7 files changed, 214 insertions(+), 63 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index a6ebbd10..76d2e637 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -20,6 +20,7 @@ jobs: version: "1.8.3" - uses: astral-sh/ruff-action@v3 with: + version: "0.15.8" args: "--version" - uses: dtolnay/rust-toolchain@stable with: diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ff84b29..1cdf4acf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ Python client generation improvements: +- Python codegen still falls back to basic formatting when Ruff is not installed, but now reports an error if `ruff format` is available and fails; pass `--format=false` to skip external formatting. +- Multi-field Rust tuple structs now generate valid Python `RootModel[tuple[...]]` models instead of invalid numeric field names. - Generated Python clients with nested namespaces now import cleanly when models refer to parent or sibling namespaces, including cases like `offer_rules.InsurerCategory`. - Namespace names containing characters that are not valid in Python identifiers, such as dashes or leading digits, now generate valid Python classes. - Schemas with a root namespace named `sys` no longer conflict with Python's standard `sys` module. diff --git a/README.md b/README.md index a11999a1..8fdbf2e9 100644 --- a/README.md +++ b/README.md @@ -42,7 +42,7 @@ ### Building and running -Ensure that you have `prettier` and `rustfmt` available in your PATH to format generated code. +Ensure that you have `prettier`, `rustfmt`, and `ruff` available in your PATH for consistently formatted generated code. To run the demo server: diff --git a/reflectapi-cli/src/main.rs b/reflectapi-cli/src/main.rs index 33bd4b2f..10c19e54 100644 --- a/reflectapi-cli/src/main.rs +++ b/reflectapi-cli/src/main.rs @@ -53,7 +53,14 @@ enum Commands { typecheck: bool, /// Format the generated code - #[arg(short, long, default_value = "true")] + #[arg( + short, + long, + default_value_t = true, + default_missing_value = "true", + num_args = 0..=1, + require_equals = true + )] format: bool, /// Instrument the generated code with tracing diff --git a/reflectapi-cli/tests/output_paths.rs b/reflectapi-cli/tests/output_paths.rs index 16a7e385..906a3edd 100644 --- a/reflectapi-cli/tests/output_paths.rs +++ b/reflectapi-cli/tests/output_paths.rs @@ -30,6 +30,15 @@ fn run(args: &[&str]) -> std::process::Output { .expect("spawn reflectapi") } +#[cfg(unix)] +fn run_with_path(args: &[&str], path: &std::path::Path) -> std::process::Output { + Command::new(cargo_bin()) + .args(args) + .env("PATH", path) + .output() + .expect("spawn reflectapi") +} + fn write_minimal_python_schema(path: &std::path::Path, type_name: &str) { let schema = format!( r#"{{ @@ -107,6 +116,7 @@ fn python_output_into_fresh_directory() { "codegen", "--language", "python", + "--format=false", "--schema", schema.to_str().unwrap(), "--output", @@ -144,6 +154,7 @@ fn python_directory_output_removes_stale_generated_files() { "codegen", "--language", "python", + "--format=false", "--schema", schema_one.to_str().unwrap(), "--output", @@ -165,6 +176,7 @@ fn python_directory_output_removes_stale_generated_files() { "codegen", "--language", "python", + "--format=false", "--schema", schema_two.to_str().unwrap(), "--output", @@ -211,6 +223,7 @@ fn python_legacy_cleanup_skips_symlinked_directories() { "codegen", "--language", "python", + "--format=false", "--schema", schema.to_str().unwrap(), "--output", @@ -229,6 +242,110 @@ fn python_legacy_cleanup_skips_symlinked_directories() { assert!(target.join("current/__init__.py").is_file()); } +#[cfg(unix)] +#[test] +fn python_format_falls_back_when_ruff_is_missing() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("python-client"); + let empty_path = tmp.path().join("bin"); + fs::create_dir_all(&empty_path).unwrap(); + + let schema = tmp.path().join("schema.json"); + write_minimal_python_schema(&schema, "current::Thing"); + + let out = run_with_path( + &[ + "codegen", + "--language", + "python", + "--schema", + schema.to_str().unwrap(), + "--output", + target.to_str().unwrap(), + ], + &empty_path, + ); + assert!( + out.status.success(), + "stderr:\n{}", + String::from_utf8_lossy(&out.stderr) + ); + assert!(target.join("current/__init__.py").is_file()); +} + +#[cfg(unix)] +#[test] +fn python_format_false_does_not_require_ruff() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("python-client"); + let empty_path = tmp.path().join("bin"); + fs::create_dir_all(&empty_path).unwrap(); + + let schema = tmp.path().join("schema.json"); + write_minimal_python_schema(&schema, "current::Thing"); + + let out = run_with_path( + &[ + "codegen", + "--language", + "python", + "--format=false", + "--schema", + schema.to_str().unwrap(), + "--output", + target.to_str().unwrap(), + ], + &empty_path, + ); + assert!( + out.status.success(), + "stderr:\n{}", + String::from_utf8_lossy(&out.stderr) + ); + assert!(target.join("current/__init__.py").is_file()); +} + +#[cfg(unix)] +#[test] +fn python_format_reports_ruff_failures() { + use std::os::unix::fs::PermissionsExt; + + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("python-client"); + let bin = tmp.path().join("bin"); + fs::create_dir_all(&bin).unwrap(); + + let ruff = bin.join("ruff"); + fs::write(&ruff, "#!/bin/sh\necho 'ruff exploded' >&2\nexit 2\n").unwrap(); + let mut permissions = fs::metadata(&ruff).unwrap().permissions(); + permissions.set_mode(0o755); + fs::set_permissions(&ruff, permissions).unwrap(); + + let schema = tmp.path().join("schema.json"); + write_minimal_python_schema(&schema, "current::Thing"); + + let out = run_with_path( + &[ + "codegen", + "--language", + "python", + "--schema", + schema.to_str().unwrap(), + "--output", + target.to_str().unwrap(), + ], + &bin, + ); + assert!( + !out.status.success(), + "expected ruff failure to fail codegen" + ); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!(stderr.contains("failed to format generated Python code with `ruff format`")); + assert!(stderr.contains("command failed with exit code")); + assert!(stderr.contains("Fix Ruff or pass `--format=false`")); +} + #[test] fn ts_output_to_file_path_writes_siblings() { // --output …/generated.ts should still work; transport file lands @@ -286,6 +403,7 @@ fn python_stdout_emits_generated_not_init() { "codegen", "--language", "python", + "--format=false", "--schema", schema.to_str().unwrap(), "--output", diff --git a/reflectapi-demo/src/tests/snapshots/reflectapi_demo__tests__basic__reflectapi_struct_tuple-5.snap b/reflectapi-demo/src/tests/snapshots/reflectapi_demo__tests__basic__reflectapi_struct_tuple-5.snap index 1223bcd1..eab3ec56 100644 --- a/reflectapi-demo/src/tests/snapshots/reflectapi_demo__tests__basic__reflectapi_struct_tuple-5.snap +++ b/reflectapi-demo/src/tests/snapshots/reflectapi_demo__tests__basic__reflectapi_struct_tuple-5.snap @@ -16,7 +16,7 @@ from __future__ import annotations from typing import Annotated, Any, Generic, Optional, TypeVar, Union # Third-party imports -from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict, Field, RootModel # Runtime imports from reflectapi_runtime import AsyncClientBase, ClientBase, ApiResponse @@ -24,12 +24,8 @@ from reflectapi_runtime import ReflectapiEmpty from reflectapi_runtime import ReflectapiInfallible -class ReflectapiDemoTestsBasicTestStructTuple(BaseModel): - - model_config = ConfigDict(extra="ignore", populate_by_name=True, protected_namespaces=(), defer_build=True) - - 0: int - 1: str +class ReflectapiDemoTestsBasicTestStructTuple(RootModel[tuple[int, str]]): + root: tuple[int, str] # Namespace classes for dotted access to types @@ -41,6 +37,7 @@ class reflectapi_demo: class basic: """Namespace for basic types.""" + TestStructTuple = ReflectapiDemoTestsBasicTestStructTuple @@ -83,7 +80,6 @@ class AsyncClient(AsyncClientBase): ) -> None: super().__init__(base_url, **kwargs) - self.inout = AsyncInoutClient(self) @@ -126,7 +122,6 @@ class Client(ClientBase): ) -> None: super().__init__(base_url, **kwargs) - self.inout = InoutClient(self) @@ -149,5 +144,8 @@ for _model in [ _rebuild_errors.append(f" - {_model.__name__}: {type(_exc).__name__}: {_exc}") if _rebuild_errors: raise RuntimeError( - "reflectapi: failed to rebuild " + str(len(_rebuild_errors)) + " generated model(s). This usually means the codegen emitted an annotation pointing at a symbol that was never defined (a dangling type reference). Fix the codegen rather than catching this error.\n" + "\n".join(_rebuild_errors) + "reflectapi: failed to rebuild " + + str(len(_rebuild_errors)) + + " generated model(s). This usually means the codegen emitted an annotation pointing at a symbol that was never defined (a dangling type reference). Fix the codegen rather than catching this error.\n" + + "\n".join(_rebuild_errors) ) diff --git a/reflectapi/src/codegen/python.rs b/reflectapi/src/codegen/python.rs index e1080396..d7379a72 100644 --- a/reflectapi/src/codegen/python.rs +++ b/reflectapi/src/codegen/python.rs @@ -43,8 +43,9 @@ pub struct Config { pub generate_sync: bool, /// Whether to generate testing utilities pub generate_testing: bool, - /// Attempt to format the generated code with ruff. Will fall back to basic - /// formatting if ruff is not available. + /// Attempt to format generated code with ruff. Falls back to basic + /// formatting when ruff is not installed, but fails if ruff is installed + /// and returns an error. pub format: bool, /// Base URL for the API (optional) pub base_url: Option, @@ -419,7 +420,7 @@ fn generate_optimized_imports(imports: &templates::Imports) -> String { third_party_imports.insert("BaseModel"); third_party_imports.insert("ConfigDict"); third_party_imports.insert("Field"); - if imports.has_discriminated_unions { + if imports.has_discriminated_unions || imports.has_tuple_structs { third_party_imports.insert("RootModel"); } if imports.has_externally_tagged_enums { @@ -1630,38 +1631,47 @@ fn build_python_generation( .types() .any(|t| matches!(t, schema_codegen::SemanticType::Enum(_))); - let (has_literal, has_discriminated_unions, has_externally_tagged_enums) = { + let (has_literal, has_discriminated_unions, has_externally_tagged_enums, has_tuple_structs) = { let mut has_literal = false; let mut has_discriminated_unions = false; let mut has_externally_tagged_enums = false; + let mut has_tuple_structs = false; for sem_type in semantic.types() { - if let schema_codegen::SemanticType::Enum(sem_enum) = sem_type { - match &sem_enum.representation { - reflectapi_schema::Representation::Internal { .. } - | reflectapi_schema::Representation::Adjacent { .. } => { - // Both internally and adjacently tagged enums now use - // Literal discriminator fields + Pydantic discriminated unions - has_literal = true; - has_discriminated_unions = true; - } - reflectapi_schema::Representation::External => { - let has_complex_variants = sem_enum - .variants - .values() - .any(|v| !matches!(v.field_style, schema_codegen::FieldStyle::Unit)); - if has_complex_variants { - has_externally_tagged_enums = true; + match sem_type { + schema_codegen::SemanticType::Enum(sem_enum) => { + match &sem_enum.representation { + reflectapi_schema::Representation::Internal { .. } + | reflectapi_schema::Representation::Adjacent { .. } => { + // Both internally and adjacently tagged enums now use + // Literal discriminator fields + Pydantic discriminated unions + has_literal = true; + has_discriminated_unions = true; + } + reflectapi_schema::Representation::External => { + let has_complex_variants = sem_enum.variants.values().any(|v| { + !matches!(v.field_style, schema_codegen::FieldStyle::Unit) + }); + if has_complex_variants { + has_externally_tagged_enums = true; + } } + _ => {} } - _ => {} } + schema_codegen::SemanticType::Struct(sem_struct) => { + if sem_struct.is_tuple && sem_struct.fields.len() > 1 { + has_tuple_structs = true; + } + } + schema_codegen::SemanticType::Primitive(_) => {} } } ( has_literal, has_discriminated_unions, has_externally_tagged_enums, + has_tuple_structs, ) }; @@ -1687,6 +1697,7 @@ fn build_python_generation( has_literal, has_discriminated_unions, has_externally_tagged_enums, + has_tuple_structs, global_type_vars: Vec::new(), // Will be added later after tracking usage has_streaming, has_partial_models, @@ -3413,6 +3424,40 @@ fn render_struct( return Ok(String::new()); } + if struct_def.is_tuple() { + let active_generics: Vec = + struct_def.parameters().map(|p| p.name.clone()).collect(); + for generic in &active_generics { + used_type_vars.insert(generic.clone()); + } + + let element_types = struct_def + .fields + .iter() + .map(|field| { + let base_field_type = type_ref_to_python_type( + &field.type_ref, + schema, + implemented_types, + class_names, + &active_generics, + used_type_vars, + )?; + let (_, _, field_type) = resolve_field_optionality( + &field.type_ref.name, + base_field_type, + field.required, + ); + Ok(field_type) + }) + .collect::, anyhow::Error>>()?; + let tuple_type = format!("tuple[{}]", element_types.join(", ")); + let class_name = python_class_name(&struct_def.name, class_names); + return Ok(format!( + "class {class_name}(RootModel[{tuple_type}]):\n root: {tuple_type}\n\n" + )); + } + // Collect active generic parameter names for this struct, mapping to descriptive names let active_generics: Vec = struct_def.parameters().map(|p| p.name.clone()).collect(); @@ -5405,36 +5450,15 @@ fn build_implemented_types() -> BTreeMap { // Helper functions for templates fn format_python_code(code: &str) -> anyhow::Result { - // Try to format with ruff format if available - use std::process::{Command, Stdio}; - - let child = Command::new("ruff") - .args(["format", "--stdin-filename", "generated.py", "-"]) - .stdin(Stdio::piped()) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .spawn(); - - match child { - Ok(mut process) => { - if let Some(stdin) = process.stdin.take() { - use std::io::Write; - let mut stdin = stdin; - let _ = stdin.write_all(code.as_bytes()); - } - let output = process.wait_with_output()?; - if output.status.success() { - let formatted = String::from_utf8_lossy(&output.stdout).to_string(); - Ok(ensure_final_newline(formatted)) - } else { - // Fall back to basic formatting if ruff fails - Ok(basic_python_format(code)) - } - } - Err(_) => { - // ruff not available, apply basic formatting - Ok(basic_python_format(code)) - } + let mut ruff = std::process::Command::new("ruff"); + ruff.args(["format", "--stdin-filename", "generated.py", "-"]); + match super::format_with([&mut ruff], code.to_string()) { + Ok(formatted) => Ok(ensure_final_newline(formatted)), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(basic_python_format(code)), + Err(err) => Err(anyhow::anyhow!( + "failed to format generated Python code with `ruff format`: {err}\n\ + Fix Ruff or pass `--format=false` to skip external formatting." + )), } } @@ -5741,6 +5765,7 @@ pub mod templates { pub has_literal: bool, pub has_discriminated_unions: bool, pub has_externally_tagged_enums: bool, + pub has_tuple_structs: bool, pub global_type_vars: Vec, /// At least one streaming endpoint is exposed by the client. pub has_streaming: bool,