Quality of life improvements for MFC toolchain #1118
+37,871
−887
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add validate command for pre-flight case validation without building - Add clean command implementation (was previously missing) - Show detailed CMake/compiler errors on build failure with formatted panels - Add build progress indicators (Configuring/Building/Installing status) - Display human-readable test names prominently in test output - Add real-time test failure feedback with helpful hints - Enhanced case validator error messages with suggestions - Add --debug-log flag for troubleshooting - Create troubleshooting documentation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Test Summary: - Rich panel with formatted pass/fail/skip counts - Total test duration display - Failed test details with UUIDs and error types - Helpful "Next Steps" suggestions for failures Case Template Generator (./mfc.sh init): - Create new cases from built-in templates (1D/2D/3D_minimal) - Copy from any example with --template example:<name> - List available templates with --list - Well-documented case files with clear parameter sections Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…oarding - Add ./mfc.sh completion command to install shell tab-completion - Installs to ~/.local/share/mfc/completions/ (works across MFC clones) - Automatically configures ~/.bashrc or ~/.zshrc - Supports install/uninstall/status subcommands - Add enhanced help output with Rich formatting - Add contextual tips after build/test failures - Add interactive menu mode (./mfc.sh interactive) - Add welcome message for first-time users with getting started guide - Add bash and zsh completion scripts - Update README with toolchain features documentation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove unused Text import from rich.text - Add pylint disable for too-many-locals in test() - Add pylint disable for too-many-arguments in _print_test_summary() - Rename failed_tests param to failed_test_list to avoid shadowing - Prefix unused skipped_cases param with underscore - Add pylint disable for global-statement in setup_debug_logging() Lint score: 10.00/10 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1. Enhanced subcommand help (./mfc.sh build --help): - Shows Rich-formatted header with command description - Displays practical examples with explanations - Lists key options in easy-to-read format - Still shows full argparse options below 2. Topic-based help (./mfc.sh help <topic>): - gpu: GPU configuration, OpenACC/OpenMP, troubleshooting - clusters: HPC cluster setup (Phoenix, Frontier, etc.) - batch: Batch job submission options and examples - debugging: Debug logging, validation, common issues 3. Command aliases for faster typing: - b = build - r = run - t = test - v = validate - c = clean 4. Consistent --help behavior: - ./mfc.sh --help now shows enhanced help (same as ./mfc.sh) - Aliases shown in command table Updated bash/zsh completion scripts to support new commands. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The 'load' command must be sourced to set environment variables in the current shell. Running it directly (./mfc.sh load) now shows a clear error message with the correct usage: source ./mfc.sh load -c p -m g Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add Unicode progress bar during first-time pip package installation Shows "│████████░░░░░░░░░░░░│ 53% (37 pkgs, 1m23s)" in terminals Falls back to milestone updates when output is piped - Defer list_cases() call in args.py until test command is actually used Previously generated all ~200 test cases on every startup - Make pyrometheus and cantera imports lazy in run/input.py These heavy chemistry packages are now only imported when needed These changes fix the apparent "hang" on first run where pip install showed no progress, and significantly speed up startup time for non-test commands. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Progress bar now tracks three phases: resolving (0-60%), building wheels (60-80%), and installing (80-100%) - Add "Starting..." message in mfc.sh for immediate user feedback - Non-TTY mode now shows phase transitions (building, installing) - Better status messages showing current operation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Display the currently processing package (collecting, downloading, or building) on a second line below the progress bar. The display updates in real-time as pip processes each package. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Use lazy imports in main.py to defer loading heavy modules - Remove heavy imports from args.py (run.run, build, test.cases) - Hardcode target names and template names to avoid import chain - Import test.cases only when running test command This reduces startup time from ~1.4s to ~1.0s by avoiding loading mako, pygments, and other heavy dependencies until they're needed. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
uv is a blazing fast Python package installer (10-100x faster than pip). When uv is detected, use it for package installation (~7 seconds vs several minutes with pip). Falls back to pip with progress bar when uv is not available. Users can install uv with: pip install uv Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Instead of requiring users to manually install uv, automatically bootstrap it in the venv using pip. This gives all users the speed benefit of uv (~23 seconds for fresh install vs several minutes with pip alone). The bootstrap process: 1. Create venv 2. Install uv via pip (~2-3 seconds) 3. Use uv to install all ~70 packages (~7 seconds) Falls back to pip with progress bar if uv installation fails. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The exact number of packages varies, so just say "Installing Python packages" without specifying a count. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Instead of hiding uv's output, show its native progress display when running in an interactive terminal. This lets users see what's happening during installation instead of appearing to hang. For non-interactive (CI) environments, output is still captured for logs. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
On HPC systems where cache and venv are on different filesystems, uv's hardlink attempts fail and fall back to copying. Setting UV_LINK_MODE=copy skips the failed hardlink attempts, reducing overhead and eliminating the warning message. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove early exit for 'init' command in mfc.sh that prevented help from showing - Fix _handle_enhanced_help to properly show both enhanced help and argparse help for all subcommands - Add subparser map to print argparse help for specific commands Now ./mfc.sh init --help (and other subcommands) properly show help. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Make build imports lazy in case.py and run/input.py to break circular import chains that were exposed by the lazy import optimizations in main.py and args.py. - case.py: lazy import build in get_inp() and get_fpp() - run/input.py: lazy import build in generate_inp(), validate_constraints(), and clean() Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Pass --help through to Python for bash-handled commands (clean, lint, format, load, spelling) so enhanced help is shown - Add missing commands to COMMANDS dict in user_guide.py: bench, completion, lint, format, spelling Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Use argparse's built-in aliases= parameter instead of separate parsers for command aliases (b, r, t, v, c). This ensures aliases inherit all arguments from their parent commands. Previously, ./mfc.sh t would fail because the 't' parser didn't have test-specific arguments like --rdma-mpi. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Parse ninja's [X/Y] output to show compile progress - Display progress bar with: file count, percentage, current file, elapsed time - Falls back to spinner with elapsed time for non-TTY environments - Shows "(build took Xs)" for builds longer than 5 seconds In interactive terminals, users now see: Building simulation [████████░░░░] 42/156 • 0:02:34 m_rhs.fpp.f90 In non-interactive mode: Building simulation... (build took 234.5s) ✓ Built simulation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The 'init' command was originally used to bootstrap the Python environment without running any command. This restores that behavior and renames the case template creation to 'new': - ./mfc.sh init -> Just bootstrap venv/environment, then exit - ./mfc.sh new -> Create a new case from a template Updated files: - mfc.sh: Restore original init behavior (exit after python.sh) - args.py: Rename init parser to new - main.py: Handle 'new' command instead of 'init' - user_guide.py: Update all references from init to new - init.py: Update usage messages to use 'new' Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When --verbose is used, bypass the progress bar and show raw cmake/compiler output. This applies to both configure and build steps. Previously, verbose mode would still capture all output in the progress bar, making the --verbose flag ineffective. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Replace black with autopep8 due to Python 3.12.5 memory safety issues - Fix bash completion to allow file path navigation after commands - Fix format.sh to accept custom path arguments - Fix pylint errors in build.py and main.py - Reformat all Python files with autopep8 for CI consistency Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Implement a declarative CLI schema that eliminates the need to manually update 8+ files when adding commands or options: - Add cli/schema.py with dataclass definitions for CLI structure - Add cli/commands.py as SINGLE SOURCE OF TRUTH for all commands - Add cli/argparse_gen.py to generate argparse parsers from schema - Add cli/completion_gen.py to generate bash/zsh completions - Add generate.py and ./mfc.sh generate command Modified files to use the schema: - args.py: Now uses generated parser from CLI schema - user_guide.py: Imports COMMANDS from cli/commands.py - Completion scripts are now auto-generated, not hand-edited Adding a new command now requires editing only cli/commands.py, then running ./mfc.sh generate to update completion scripts. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Extract helper functions to reduce complexity in completion_gen.py and argparse_gen.py. Add pylint disable for dataclasses that legitimately need many attributes. Code now scores 10.00/10 on pylint. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add cli/docs_gen.py to generate CLI reference markdown from schema - Update generate.py to also produce docs/cli-reference.md - Add generate --check step to lint-toolchain CI workflow - Generated documentation includes quick reference table, all commands with options, examples, and common options section Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This adds toolchain/mfc/params_tests/ with tools to capture and verify validation behavior before refactoring case_validator.py: - inventory.py: Export all 3300 parameters with types and stages - snapshot.py: Capture validation results from 134 example cases - coverage.py: Analyze 306 constraints across 56 check methods - runner.py: CLI for build/verify/summary commands Run `python -m mfc.params_tests.runner build` to capture baseline, then `python -m mfc.params_tests.runner verify` after changes to detect any unintended changes to validation behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Extends the safety net with two new testing approaches: 1. negative_tests.py - Hand-crafted test cases that intentionally violate constraints (20 tests, 100% trigger rate) 2. mutation_tests.py - Automatically mutates valid example cases to test validator coverage (830 mutations, 82.8% catch rate) Mutation testing found real validator gaps: - t_step_save: No validation for 0 or negative values - mpp_lim/cyl_coord: Invalid strings like "X" not caught - x_domain%beg/end: Missing value not validated Run with: python -m mfc.params_tests.runner negative python -m mfc.params_tests.runner mutation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Mutation testing found several gaps where the validator allowed invalid parameter values. This commit fixes them: 1. Add check_parameter_types() - validates Fortran logicals are 'T'/'F' - mpp_lim, cyl_coord, bubbles_euler, etc. now reject 'X', 'yes', etc. 2. Add x_domain validation - x_domain%beg/end required when m > 0 3. Fix t_step_save - now validates t_step_save > 0 4. Fix dt validation - now validates dt > 0 in all modes where dt is set 5. Improve time stepping validation - properly handles cfl_dt, cfl_adap_dt, and adap_dt modes Mutation test results improved from 82.8% to 99.9% catch rate. All 134 example cases still pass validation. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Implement a central parameter registry that serves as the single source of truth for MFC's ~3,300 case parameters. This eliminates manual synchronization between case_dicts.py, case_validator.py, and documentation. Key changes: - Create mfc/params/ package with schema definitions and registry - Refactor case_dicts.py from ~680 lines to ~115 lines (uses registry) - Add parameter definitions organized by domain (core, weno, patches, etc.) - Add code generators for case_dicts, validator, and documentation - Add physics validation for patches, acoustics, and bubbles The registry provides: - ParamDef dataclass for parameter metadata (name, type, stages, description) - ConstraintRule dataclass for validation constraints (26 defined) - Generators that produce case_dicts-compatible output - Documentation generator (~194K chars of markdown) - Comparison tool to verify registry matches original All tests pass: - Safety net: 134/134 cases unchanged - Mutation tests: 99.9% catch rate - JSON schema validation: working Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:XXL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Contributor
|
CodeAnt AI Incremental review completed. |
The % character is special in Doxygen. Using %% renders as a literal %. This fixes parameter names like bc_y%alpha_in showing as bc_yalpha_in. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Instead of listing every combination like (1,1), (1,2), (2,1), etc., the docs now show a single pattern like (N, M). Before: 78 rows for simplex_params (showing every index combination) After: 8 rows showing unique patterns Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="docs/documentation/parameters.md">
<violation number="1" location="docs/documentation/parameters.md:63">
P2: The documentation now lists parameters with `%%` (e.g., `patch_icpp(N)%%Bx`), which appears to be a typo and misdocuments the actual case file syntax (`%`). This will mislead users copying parameter names.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Previously, small families (<20 params) showed full tables even when they were indexed. Now we always collapse patterns if it reduces rows. Example: schlieren_alpha with 10 params now shows 1 pattern row instead of 10 individual rows. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@toolchain/mfc/params/generators/docs_gen.py`:
- Line 137: The type annotation for the variable families incorrectly uses the
builtin any instead of typing.Any; change the annotation from Dict[str,
List[Tuple[str, any]]] to Dict[str, List[Tuple[str, Any]]] and ensure Any is
imported from typing (add or update the import statement that provides Any) so
the annotation references typing.Any; locate the declaration of families in
docs_gen.py to apply the fix.
- Around line 49-74: The nested block that handles parenthesized numeric indices
inside the while i < len(name) loop should be extracted into a helper function
to reduce nesting and fix R1702; create a function (e.g.,
handle_parenthesized_indices or parse_parenthesized_indices) that takes (name,
start_index, placeholders, placeholder_idx), moves the inner logic that collects
paren_content, validates numeric parts, builds new_parts and returns the
replacement string (like '(?, ?)' or with placeholders), the updated index j+1,
and the updated placeholder_idx; then replace the inlined code in the main loop
with a single call to that helper and append the returned segment to result or
fall back to the original flow when the helper indicates no valid parenthesized
numeric indices.
🧹 Nitpick comments (2)
toolchain/mfc/params/generators/docs_gen.py (2)
15-15: Remove unusednoqadirective.Ruff reports the
# noqa: F401comment is unnecessary since no F401 warning is being suppressed. The import is actually used (for side effects of registering definitions).🧹 Proposed fix
-from .. import definitions # noqa: F401 pylint: disable=unused-import +from .. import definitions # pylint: disable=unused-import
24-26: Simplify unnecessaryany()over single-element list.The expression
any(... for _ in [1])iterates over a single element, makingany()pointless. This looks like a refactoring leftover.🧹 Proposed fix
- if any(name.startswith(f"{base}(") or name.startswith(f"{base}%") for _ in [1]): + if name.startswith(f"{base}(") or name.startswith(f"{base}%"): return base
- Change `any` to `Any` from typing module - Extract _parse_paren_content helper to reduce nesting in _collapse_indices - Fixes R1702 pylint warning (too many nested blocks) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@docs/documentation/parameters.md`:
- Around line 165-180: Several fluid_pp parameters (`fluid_pp(N)%%D_v`, `%%M_v`,
`%%cp_v`, `%%gamma_v`, `%%gamma_v`, `%%k_v`, `%%mu_v`, `%%mul0`, `%%pv`, `%%ss`)
and general-family params (`Ca`, `Re_inv`, `Web`) are missing human-readable
descriptions in the parameter registry; open the parameter descriptions registry
(toolchain/mfc/params/descriptions.py) and add concise, validated descriptions
and units for each symbol (e.g., D_v = vapor diffusivity [m^2/s], M_v = vapor
molecular weight [kg/mol], cp_v = vapor specific heat at constant pressure
[J/(kg·K)], gamma_v = vapor heat capacity ratio, k_v = vapor thermal
conductivity [W/(m·K)], mu_v = vapor dynamic viscosity [Pa·s], mul0 = reference
viscosity, pv = vapor pressure, ss = surface tension source/flag or clarify
exact meaning, Ca = Capillary number, Re_inv = inverse Reynolds number, Web =
Weber number), verifying physical meanings against source physics before
committing so the generated docs populate correctly.
In `@toolchain/mfc/params/generators/docs_gen.py`:
- Around line 233-242: The description text returned by get_description(example)
isn't escaped for percent signs before being appended to lines, so call the
existing _escape_percent on the (possibly truncated) desc variable before using
it in lines.append to prevent Doxygen format tokens; update the block that
builds the table row (the loop iterating over patterns/examples that sets desc,
truncates it, and appends to lines) to replace desc with _escape_percent(desc)
and apply the identical change to the other similar block later in the file that
builds example rows (the second block that handles examples/patterns around the
same codepath).
- Line 15: Remove the unused "noqa: F401" directive on the import statement in
docs_gen.py: keep the intentional side-effect import "from .. import
definitions" and retain the "pylint: disable=unused-import" comment but delete
the "# noqa: F401" portion so Ruff no longer flags an unused noqa; ensure the
line still imports definitions for side effects.
- Add descriptions for dimensionless numbers: Ca (Cavitation number), Web (Weber number), Re_inv (Inverse Reynolds number) - Add descriptions for fluid_pp vapor phase parameters: mul0, ss, pv, gamma_v, M_v, mu_v, k_v, cp_v, D_v - Add detailed descriptions for bub_pp (sub-grid bubble) parameters including thermal properties, viscosities, and gas constants Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:XXL
Contributor
|
CodeAnt AI Incremental review completed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@toolchain/mfc/params/descriptions.py`:
- Around line 566-583: The description normalization in get_pattern_description
leaves numeric indices inside parentheses (e.g., "a(2)", "offset (2)")
unchanged; update get_pattern_description to also replace parenthesized example
indices by mapping "(1)"/"(2)"/"(3)" to "(N)"/"(M)"/"(K)" (use the existing
example variable created from pattern_name) — add regex substitutions targeting
"\(1\)/\(2\)/\(3\)" to "\(N\)/\(M\)/\(K\)" (run these alongside the existing
space-separated replacements so both "component 2" and "component(2)" or "offset
(2)" become generic).
Normalize (1)/(2)/(3) to (N)/(M)/(K) in get_pattern_description to prevent example values like "a(2)" from appearing in pattern docs. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add specific descriptions for 17 Lagrangian output fields (lag_*_wrt) - Add specific descriptions for 17 lag_params fields with physics context - Add specific descriptions for 4 chem_params fields - All 3,400 parameters now have meaningful descriptions (100% coverage) Descriptions derived from Fortran source comments in m_derived_types.fpp. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/documentation/parameters.md`:
- Around line 61-70: The table entries show double-escaped percent signs (e.g.,
"patch_icpp(N)%%Bx", "patch_icpp(1)%%alpha(1)") but the real syntax uses a
single '%' (e.g., "patch_icpp(1)%geometry"); update the doc-generator/template
that produces these parameter patterns to emit a single '%' instead of '%%'
(locate the generation/escaping routine that outputs strings like
"patch_icpp(N)%%Bx" and change the escaping or formatting so '%' is not
doubled), ensure examples (e.g., "patch_icpp(1)%%By", "patch_icpp(1)%%alpha")
are corrected to use a single '%', and add/adjust a unit test or snapshot for
the generator to assert that patterns contain single '%' characters.
🧹 Nitpick comments (1)
toolchain/mfc/params/descriptions.py (1)
517-520: Preserve suffixes if a name ever contains multiple%.
split("%")drops any later segments;split("%", 1)keeps the full suffix and is more robust.♻️ Suggested change
- parts = name.split("%") - prefix = parts[0] - suffix = parts[1] + prefix, suffix = name.split("%", 1)
Addresses issues from code review: Critical fixes: - Fix import path bug in case_dicts_gen.py (mfc -> ...run) - Add registry freeze() to prevent post-init mutation - Validate ParamDef stages is non-empty Improvements: - Integrate case_validator.py with registry for LOG params (362 params) - Add schema validation for CONSTRAINTS/DEPENDENCIES dicts - Add type system sync verification between params and case_dicts - Add consistent error message formatting (errors.py) - Improve error handling in definitions._init_registry() - Narrow exception handling in snapshot.py and coverage.py Testing: - Add 75 unit tests covering registry, definitions, integration - Add CLI smoke tests Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@toolchain/mfc/cli/test_cli.py`:
- Around line 62-63: The two assertions in test_cli.py that reference cmd.name
use unnecessary f-strings; update the assertion messages in the test (the calls
to self.assertIsNotNone and self.assertTrue that reference cmd.name) to use
plain string literals instead of f-prefixed strings (e.g., change f"Command
missing name" and f"Command has empty name" to "Command missing name" and
"Command has empty name") so there are no needless f-string prefixes.
In `@toolchain/mfc/params_tests/test_definitions.py`:
- Around line 73-78: Add spell-check ignore directives for the intentional typos
used in these tests so the CI spell checker won't fail: update the test function
test_invalid_key_raises and other tests that call _validate_constraint (the
occurrences of "choises", "when_tru", and "requirs") to include the repo's
Spellcheck ignore comment immediately next to those literal typos (and apply the
same fix to the other occurrences noted in the file). Ensure each
intentional-misspelling token has a spell-check exclusion comment so the tests
continue to validate error handling without CI spellcheck failures.
- Around line 111-125: The tests test_invalid_top_level_key_raises and
test_invalid_condition_key_raises use intentional typos ("when_tru" and
"requirs") that trigger spellcheck; add cspell ignore directives so the linter
won't flag them—for example place a single-line comment like "# cspell:ignore
tru requirs" immediately above or on the same lines containing the typos in the
test functions that call _validate_dependency, referencing the exact
misspellings "when_tru" and "requirs".
In `@toolchain/mfc/params_tests/test_validate.py`:
- Around line 93-100: Two tests unpack the tuple returned by validate_case but
don't use one of the values; update the unpacking in the tests (e.g., in the
test that calls validate_case(params) and the test_warn_false_skips_warnings
that calls validate_case(params, warn=False)) to prefix unused variables with an
underscore (use _errors or _warnings) so the linter knows the unused value is
intentional while leaving the used variable name intact.
- Around line 14-16: Remove the unused imports ParamDef, ParamType, and Stage by
deleting the line that imports them from schema; keep the REGISTRY import as-is.
Also remove the trailing " # noqa: F401" from the "from ..params import
definitions # noqa: F401 Populate registry" line so the file has no unused
noqa directive while still importing definitions to populate the registry.
In `@toolchain/mfc/params/definitions.py`:
- Line 8: Remove the unused typing imports List and Union from the module import
line (currently importing Dict, List, Any, Union) so the statement only imports
the types actually used (Dict and Any); update the import in definitions.py (the
import that names Dict/List/Any/Union) and run tests/lint to ensure no remaining
references to List or Union remain elsewhere in the file.
- Line 472: Update the inline comment that currently reads "# This catches typos
like "choises" instead of "choices"" to correct the misspelling "choises" to
"choices" (or rephrase the comment to avoid demonstrating the typo directly), so
the spell checker no longer flags it; look for that exact comment string in
definitions.py and replace it with the corrected wording while keeping the
explanatory intent.
- Line 27: Update the comment line that currently reads: These validation
functions catch typos like "choises" instead of "choices" to avoid the
spell-check flag; either fix the example by changing "choises" → "choices" or
reword to explicitly label the misspelling (e.g., mention "'choises'
(misspelling of 'choices')") so the intent remains clear but the spell checker
no longer flags the file; target the comment in definitions.py that contains the
example typo.
In `@toolchain/mfc/params/errors.py`:
- Line 22: The import list in this module includes an unused symbol "Union";
remove "Union" from the typing import statement so it reads only the actually
used types (e.g., Any, List, Optional) in the import line at the top of
errors.py to eliminate the unused-import warning.
In `@toolchain/mfc/params/registry.py`:
- Around line 35-37: The class RegistryFrozenError has an unnecessary pass
statement; remove the redundant "pass" from the class body so the docstring
alone defines the empty subclass of RuntimeError (look for the
RegistryFrozenError class in registry.py and delete the pass line).
🧹 Nitpick comments (12)
toolchain/mfc/params_tests/coverage.py (2)
55-60: Return type annotation should includeNone.The function can return
None(line 60), but the return type is annotated asast.ClassDef. This should beast.ClassDef | Nonefor accurate type checking.Proposed fix
-def _find_case_validator_class(tree: ast.Module) -> ast.ClassDef: +def _find_case_validator_class(tree: ast.Module) -> ast.ClassDef | None: """Find the CaseValidator class in the AST."""
280-290: Fix implicitOptionaltype annotation.Per PEP 484 and the static analysis hint (Ruff RUF013), the parameter type should explicitly include
None. Additionally, the return type annotation is missing.Proposed fix
-def save_coverage_report(output_path: Path = None): +def save_coverage_report(output_path: Path | None = None) -> Path: """Save coverage report to JSON file."""toolchain/mfc/cli/test_cli.py (1)
92-95: Narrow the exception type informat_help()test.Catching a broad
Exceptionmasks potential programming errors. Sinceformat_help()typically raisesargparseerrors, consider a narrower catch or useself.fail()only for expected failure scenarios.♻️ Proposed fix
# Parser should not raise error when printing help - try: - parser.format_help() - except Exception as e: - self.fail(f"Parser help failed: {e}") + # This should not raise - if it does, the test framework will catch it + help_text = parser.format_help() + self.assertIsInstance(help_text, str)toolchain/mfc/params_tests/snapshot.py (2)
135-135: Use explicitOptionaltype hint.PEP 484 recommends explicit
Optional[Path]instead ofPath = None.♻️ Proposed fix
-def capture_all_examples(examples_dir: Path = None) -> Dict[str, CaseSnapshot]: +def capture_all_examples(examples_dir: Optional[Path] = None) -> Dict[str, CaseSnapshot]:Add
Optionalto the imports at line 12:-from typing import Dict, Any, List, Optional +from typing import Dict, Any, List, Optional(Already imported, just need to use it in the signature)
186-186: Use explicitOptionaltype hint for consistency.♻️ Proposed fix
-def save_snapshots(snapshots: Dict[str, CaseSnapshot], output_path: Path = None): +def save_snapshots(snapshots: Dict[str, CaseSnapshot], output_path: Optional[Path] = None):toolchain/mfc/params_tests/test_registry.py (1)
121-128: Avoid redefiningRegistryFrozenErrorfrom outer scope.The import at line 123 redefines
RegistryFrozenErrorwhich is already imported at line 8. While this works, it's cleaner to use the already-imported name.♻️ Proposed fix
def test_global_registry_cannot_be_modified(self): """Global REGISTRY should reject new registrations.""" - from ..params import REGISTRY, RegistryFrozenError + from ..params import REGISTRY with self.assertRaises(RegistryFrozenError): REGISTRY.register( ParamDef(name="injected", param_type=ParamType.INT, stages={Stage.COMMON}) )toolchain/mfc/params/__init__.py (2)
27-27: Remove unusednoqadirective.The
noqa: F401directive appears to be unnecessary. If the linter is configured to ignore unused imports in__init__.pyfiles, this can be removed.♻️ Proposed fix
-from . import definitions # noqa: F401 pylint: disable=unused-import +from . import definitions # pylint: disable=unused-import
29-29: Consider sorting__all__alphabetically.Sorting helps with readability and reduces merge conflicts.
♻️ Proposed fix
-__all__ = ['REGISTRY', 'RegistryFrozenError', 'ParamDef', 'ParamType', 'Stage'] +__all__ = ['ParamDef', 'ParamType', 'REGISTRY', 'RegistryFrozenError', 'Stage']toolchain/mfc/params/definitions.py (1)
481-486: Consider narrowing exception handling for initialization.The broad
Exceptioncatch could mask unexpected errors. Consider catching more specific exceptions that could reasonably occur during initialization.♻️ Proposed fix
- except Exception as e: + except (ValueError, TypeError, KeyError) as e: # Re-raise with context to help debugging initialization failures raise RuntimeError( f"Failed to initialize parameter registry: {e}\n" "This is likely a bug in the parameter definitions." ) from eThis is optional since the current implementation does preserve the original exception via
from e, making debugging still possible.toolchain/mfc/params/generators/case_dicts_gen.py (1)
94-94: Remove unused noqa directive.The
# noqa: F401directive is flagged as unused by the linter.🧹 Proposed fix
- from .. import definitions # noqa: F401 pylint: disable=unused-import + from .. import definitions # pylint: disable=unused-importtoolchain/mfc/params/registry.py (1)
56-61: Consider usingOptionalfor the type hint.The
_params_proxyis initialized toNonebut typed asMapping[str, ParamDef]. UsingOptionalwould be more precise.🔧 Proposed fix
+from typing import Dict, Set, Mapping, Optional -from typing import Dict, Set, Mapping def __init__(self): """Initialize an empty registry.""" self._params: Dict[str, ParamDef] = {} self._by_stage: Dict[Stage, Set[str]] = defaultdict(set) self._frozen: bool = False - self._params_proxy: Mapping[str, ParamDef] = None + self._params_proxy: Optional[Mapping[str, ParamDef]] = Nonetoolchain/mfc/params/validate.py (1)
40-40: Remove unused noqa directive.The
# noqa: F401directive is flagged as unused by the linter.🧹 Proposed fix
-from . import definitions # noqa: F401 pylint: disable=unused-import +from . import definitions # pylint: disable=unused-import
Comment on lines
+62
to
+63
| self.assertIsNotNone(cmd.name, f"Command missing name") | ||
| self.assertTrue(len(cmd.name) > 0, f"Command has empty name") |
Contributor
There was a problem hiding this comment.
Remove extraneous f prefix from strings without placeholders.
These f-strings have no placeholders, making the f prefix unnecessary.
🔧 Proposed fix
- self.assertIsNotNone(cmd.name, f"Command missing name")
- self.assertTrue(len(cmd.name) > 0, f"Command has empty name")
+ self.assertIsNotNone(cmd.name, "Command missing name")
+ self.assertTrue(len(cmd.name) > 0, "Command has empty name")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| self.assertIsNotNone(cmd.name, f"Command missing name") | |
| self.assertTrue(len(cmd.name) > 0, f"Command has empty name") | |
| self.assertIsNotNone(cmd.name, "Command missing name") | |
| self.assertTrue(len(cmd.name) > 0, "Command has empty name") |
🧰 Tools
🪛 Ruff (0.14.14)
[error] 62-62: f-string without any placeholders
Remove extraneous f prefix
(F541)
[error] 63-63: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
In `@toolchain/mfc/cli/test_cli.py` around lines 62 - 63, The two assertions in
test_cli.py that reference cmd.name use unnecessary f-strings; update the
assertion messages in the test (the calls to self.assertIsNotNone and
self.assertTrue that reference cmd.name) to use plain string literals instead of
f-prefixed strings (e.g., change f"Command missing name" and f"Command has empty
name" to "Command missing name" and "Command has empty name") so there are no
needless f-string prefixes.
Comment on lines
73
to
78
| def test_invalid_key_raises(self): | ||
| """Invalid constraint key should raise ValueError.""" | ||
| with self.assertRaises(ValueError) as ctx: | ||
| _validate_constraint("test", {"choises": [1, 2]}) # Typo | ||
|
|
||
| self.assertIn("choises", str(ctx.exception)) |
Contributor
There was a problem hiding this comment.
Add spell-check exceptions for intentional typos in tests.
The pipeline spell checker is flagging the intentional typos (choises, when_tru, requirs) used to test error handling. Add spell-check ignore directives to prevent CI failures.
🔧 Proposed fix
def test_invalid_key_raises(self):
"""Invalid constraint key should raise ValueError."""
with self.assertRaises(ValueError) as ctx:
- _validate_constraint("test", {"choises": [1, 2]}) # Typo
+ _validate_constraint("test", {"choises": [1, 2]}) # Typo # cspell:ignore choises
- self.assertIn("choises", str(ctx.exception))
+ self.assertIn("choises", str(ctx.exception)) # cspell:ignore choisesApply similar fixes to lines 114, 116, 122, and 125 for tru and requirs.
🧰 Tools
🪛 GitHub Actions: Spell Check
[error] 76-76: choises should be choices, chooses
[error] 78-78: choises should be choices, chooses
🤖 Prompt for AI Agents
In `@toolchain/mfc/params_tests/test_definitions.py` around lines 73 - 78, Add
spell-check ignore directives for the intentional typos used in these tests so
the CI spell checker won't fail: update the test function
test_invalid_key_raises and other tests that call _validate_constraint (the
occurrences of "choises", "when_tru", and "requirs") to include the repo's
Spellcheck ignore comment immediately next to those literal typos (and apply the
same fix to the other occurrences noted in the file). Ensure each
intentional-misspelling token has a spell-check exclusion comment so the tests
continue to validate error handling without CI spellcheck failures.
Comment on lines
111
to
125
| def test_invalid_top_level_key_raises(self): | ||
| """Invalid top-level dependency key should raise.""" | ||
| with self.assertRaises(ValueError) as ctx: | ||
| _validate_dependency("test", {"when_tru": {"requires": []}}) # Typo | ||
|
|
||
| self.assertIn("when_tru", str(ctx.exception)) | ||
|
|
||
| def test_invalid_condition_key_raises(self): | ||
| """Invalid condition key should raise.""" | ||
| with self.assertRaises(ValueError) as ctx: | ||
| _validate_dependency("test", { | ||
| "when_true": {"requirs": ["foo"]} # Typo | ||
| }) | ||
|
|
||
| self.assertIn("requirs", str(ctx.exception)) |
Contributor
There was a problem hiding this comment.
Same spell-check issue for dependency validation tests.
Add # cspell:ignore directives for tru and requirs.
🔧 Proposed fix
def test_invalid_top_level_key_raises(self):
"""Invalid top-level dependency key should raise."""
with self.assertRaises(ValueError) as ctx:
- _validate_dependency("test", {"when_tru": {"requires": []}}) # Typo
+ _validate_dependency("test", {"when_tru": {"requires": []}}) # Typo # cspell:ignore tru
- self.assertIn("when_tru", str(ctx.exception))
+ self.assertIn("when_tru", str(ctx.exception)) # cspell:ignore tru
def test_invalid_condition_key_raises(self):
"""Invalid condition key should raise."""
with self.assertRaises(ValueError) as ctx:
_validate_dependency("test", {
- "when_true": {"requirs": ["foo"]} # Typo
+ "when_true": {"requirs": ["foo"]} # Typo # cspell:ignore requirs
})
- self.assertIn("requirs", str(ctx.exception))
+ self.assertIn("requirs", str(ctx.exception)) # cspell:ignore requirs🧰 Tools
🪛 GitHub Actions: Spell Check
[error] 114-114: tru should be thru, true
[error] 116-116: tru should be thru, true
[error] 122-122: requirs should be requires
[error] 125-125: requirs should be requires
🤖 Prompt for AI Agents
In `@toolchain/mfc/params_tests/test_definitions.py` around lines 111 - 125, The
tests test_invalid_top_level_key_raises and test_invalid_condition_key_raises
use intentional typos ("when_tru" and "requirs") that trigger spellcheck; add
cspell ignore directives so the linter won't flag them—for example place a
single-line comment like "# cspell:ignore tru requirs" immediately above or on
the same lines containing the typos in the test functions that call
_validate_dependency, referencing the exact misspellings "when_tru" and
"requirs".
toolchain/mfc/params/definitions.py
Outdated
| # ============================================================================ | ||
| # Schema Validation for Constraints and Dependencies | ||
| # ============================================================================ | ||
| # These validation functions catch typos like "choises" instead of "choices" |
Contributor
There was a problem hiding this comment.
Fix typo: "choises" → "choices".
The spell check flagged this typo in the comment.
🔧 Proposed fix
-# These validation functions catch typos like "choises" instead of "choices"
+# These validation functions catch typos like "choises" instead of "choices"Wait, looking more carefully - the comment is intentionally showing "choises" as an example of a typo that would be caught. However, the spell checker still flags it. Consider rewording:
-# These validation functions catch typos like "choises" instead of "choices"
+# These validation functions catch typos (e.g., misspelled constraint keys)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # These validation functions catch typos like "choises" instead of "choices" | |
| # These validation functions catch typos (e.g., misspelled constraint keys) |
🧰 Tools
🪛 GitHub Actions: Spell Check
[error] 27-27: choises should be choices, chooses
🤖 Prompt for AI Agents
In `@toolchain/mfc/params/definitions.py` at line 27, Update the comment line that
currently reads: These validation functions catch typos like "choises" instead
of "choices" to avoid the spell-check flag; either fix the example by changing
"choises" → "choices" or reword to explicitly label the misspelling (e.g.,
mention "'choises' (misspelling of 'choices')") so the intent remains clear but
the spell checker no longer flags the file; target the comment in definitions.py
that contains the example typo.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replaces manual typo entries with automatic fuzzy matching against MFC-recognized parameter names, constraint keys, and dependency keys. Changes: - Add rapidfuzz dependency to pyproject.toml - Create suggest.py with fuzzy matching utilities using rapidfuzz - Update definitions.py to use suggest.py for constraint/dependency validation - Add unknown_param_error() to errors.py for consistent error formatting - Add check_unknown_params() to validate.py for unknown parameter detection - Update validate_case() with check_unknown parameter - Add comprehensive tests for 'did you mean?' functionality - Update .typos.toml with intentional test typo values Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@toolchain/mfc/cli/test_cli.py`:
- Around line 93-96: Remove the broad try/except around parser.format_help in
the test and instead assert its return value or simply call it so any exception
will fail the test; update the test method (the block containing
parser.format_help) to either use an assert on the returned help string (e.g.,
assert parser.format_help() is not None or assert
isinstance(parser.format_help(), str)) or just call parser.format_help() without
catching Exception to eliminate the Ruff BLE001 warning and preserve failure
behavior.
In `@toolchain/mfc/params/suggest.py`:
- Around line 84-91: The code in the suggestions formatter uses an unnecessary
else after a return (the block that starts with "else:" after checking
len(suggestions) == 1); remove that else and de-indent its body so the branch
that builds quoted = [f"'{s}'" for s in suggestions] and returns the "Did you
mean one of..." string executes directly after the return, keeping the early
return for the single-suggestion case; update the block surrounding the variable
suggestions and the conditional that returns f"Did you mean '{suggestions[0]}'?"
accordingly.
In `@toolchain/mfc/params/validate.py`:
- Line 42: The import line "from . import definitions" in validate.py currently
has an unnecessary "noqa: F401" directive; remove the "noqa: F401" while keeping
the existing "pylint: disable=unused-import" comment so the unused-import
suppression remains, i.e., update the import statement that references
definitions to drop only the "noqa: F401" token.
🧹 Nitpick comments (5)
toolchain/mfc/params_tests/test_registry.py (1)
117-120: Consider extracting the parameter count threshold.The hardcoded
3000works for now but could become brittle if the parameter count changes significantly. Consider using a named constant or a more flexible assertion (e.g.,assertGreater(..., 1000)as a sanity check).toolchain/mfc/params/suggest.py (1)
94-106: Import inside function is acceptable here.The pipeline flags
import-outside-toplevelat line 104, but this is a deliberate pattern to avoid circular imports betweensuggest.pyandregistry.py. Consider adding a brief comment explaining this if not already obvious from context.toolchain/mfc/params/definitions.py (2)
36-36: Import inside function is intentional (pipeline warning).The pipeline flags
import-outside-toplevelat lines 36 and 61. This is a deliberate pattern to avoid circular imports betweendefinitions.pyandsuggest.py. The pattern is acceptable given the module structure.Also applies to: 61-61
302-303: Complex ternary reduces readability.This line combines multiple conditions in a single expression, making it difficult to parse:
_r(n, LOG if "wrt" in n or n in ["alt_soundspeed", "mixture_err"] else (INT if n == "fd_order" else REAL), {POST})Consider splitting into explicit conditionals for clarity.
♻️ Proposed refactor
- for n in ["prim_vars_wrt", "alt_soundspeed", "mixture_err", "t_stop", "t_save", "fd_order"]: - _r(n, LOG if "wrt" in n or n in ["alt_soundspeed", "mixture_err"] else (INT if n == "fd_order" else REAL), {POST}) + # POST_PROCESS params with mixed types + for n in ["prim_vars_wrt", "alt_soundspeed", "mixture_err"]: + _r(n, LOG, {POST}) + for n in ["t_stop", "t_save"]: + _r(n, REAL, {POST}) + _r("fd_order", INT, {POST})toolchain/mfc/params/errors.py (1)
137-153: Minor grammar issue when only one conflicting parameter.When
activecontains a single element, the message "are all enabled" is grammatically awkward (e.g., "'b' are all enabled"). Consider handling singular vs plural.✏️ Proposed fix for grammar
def mutual_exclusion_error(params: List[str], active: List[str]) -> str: """ Create a mutual exclusion error message. Args: params: List of mutually exclusive parameters active: List of parameters that are currently active (conflicting) Returns: Formatted error message. """ formatted = [format_param(p) for p in params] active_formatted = [format_param(p) for p in active] - return ( - f"Only one of {', '.join(formatted)} can be enabled, " - f"but {', '.join(active_formatted)} are all enabled" - ) + verb = "is" if len(active) == 1 else "are" + return ( + f"Only one of {', '.join(formatted)} can be enabled, " + f"but {', '.join(active_formatted)} {verb} enabled" + )
Comment on lines
+93
to
+96
| try: | ||
| parser.format_help() | ||
| except Exception as e: | ||
| self.fail(f"Parser help failed: {e}") |
Contributor
There was a problem hiding this comment.
Avoid catching Exception in tests; assert on the return instead.
parser.format_help() already raises if broken; the try/except adds a Ruff BLE001 warning.
Proposed fix
- try:
- parser.format_help()
- except Exception as e:
- self.fail(f"Parser help failed: {e}")
+ self.assertIsInstance(parser.format_help(), str)🧰 Tools
🪛 Ruff (0.14.14)
[warning] 95-95: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@toolchain/mfc/cli/test_cli.py` around lines 93 - 96, Remove the broad
try/except around parser.format_help in the test and instead assert its return
value or simply call it so any exception will fail the test; update the test
method (the block containing parser.format_help) to either use an assert on the
returned help string (e.g., assert parser.format_help() is not None or assert
isinstance(parser.format_help(), str)) or just call parser.format_help() without
catching Exception to eliminate the Ruff BLE001 warning and preserve failure
behavior.
| # Note: definitions is imported by params/__init__.py to populate REGISTRY. | ||
| # This redundant import ensures REGISTRY is populated even if this module | ||
| # is imported directly (e.g., during testing). | ||
| from . import definitions # noqa: F401 pylint: disable=unused-import |
Contributor
There was a problem hiding this comment.
Remove unused noqa directive (static analysis hint).
Ruff flags the noqa: F401 as unused since F401 isn't enabled in its configuration. The pylint: disable=unused-import comment is sufficient.
🧹 Proposed fix
-from . import definitions # noqa: F401 pylint: disable=unused-import
+from . import definitions # pylint: disable=unused-import📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| from . import definitions # noqa: F401 pylint: disable=unused-import | |
| from . import definitions # pylint: disable=unused-import |
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 42-42: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
🤖 Prompt for AI Agents
In `@toolchain/mfc/params/validate.py` at line 42, The import line "from . import
definitions" in validate.py currently has an unnecessary "noqa: F401" directive;
remove the "noqa: F401" while keeping the existing "pylint:
disable=unused-import" comment so the unused-import suppression remains, i.e.,
update the import statement that references definitions to drop only the "noqa:
F401" token.
- Remove unnecessary else after return in format_suggestion() - Add pylint disable comments for intentional import-outside-toplevel (needed to avoid circular imports at module load time) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The test uses 'when_tru' as an intentional typo to test dependency key validation with 'did you mean?' suggestions. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
User description
PR Type
Enhancement, Tests, Documentation
Description
Major Quality of Life Improvements for MFC Toolchain
Centralized Parameter System: Consolidated ~3,300 MFC case parameters into unified registry with descriptions, constraints, and dependencies (
params/definitions.py,params/descriptions.py)Enhanced CLI Architecture: Refactored argument parsing with declarative schema-based command definitions, auto-generated argparse parsers, and shell completion support (
cli/schema.py,cli/argparse_gen.py,cli/completion_gen.py)Improved User Experience: Added interactive user guide with topic-based help system, contextual tips, and first-time user onboarding (
user_guide.py,completion.py)Case Template System: New case template generator with built-in minimal templates and example-based case creation (
init.py)Parameter Discovery: New CLI command for searching and exploring parameters with filtering and statistics (
params_cmd.py)Enhanced Validation: Comprehensive physics constraint validation for bubbles, patches, time stepping, and acoustic sources with improved error reporting (
case_validator.py)Build System Improvements: Added progress bars, streaming output, and detailed error reporting with contextual hints (
build.py)Test Infrastructure: Comprehensive parameter validation test suite including negative tests, mutation testing, snapshot regression testing, and coverage analysis (
params_tests/module)Documentation Generation: Auto-generated CLI reference and parameter documentation from schema definitions (
cli/docs_gen.py,params/generators/docs_gen.py)Code Quality: Fixed formatting issues across example cases and benchmarks (PEP 8 compliance for imports and comments)
Diagram Walkthrough
File Walkthrough
15 files
descriptions.py
Centralized parameter descriptions and documentation systemtoolchain/mfc/params/descriptions.py
case parameters
patch_icpp(N)%geometry)parameters
case_dicts.py
Migrate parameter definitions to central registrytoolchain/mfc/run/case_dicts.py
of hardcoded dictionaries
loading via
_load_from_registry()CASE_OPTIMIZATIONto derive from registry metadata instead ofhardcoded list
ParamTypeenum and utility functionscommands.py
Unified CLI command definitions and schematoolchain/mfc/cli/commands.py
etc.) with full argument specifications
completions, and documentation
case_validator.py
Enhanced case validation with physics constraintstoolchain/mfc/case_validator.py
check_parameter_types()to validate logical parameters are 'T'or 'F'
check_bubbles_euler()with validation for bubble physicsparameters (R0ref, p0ref, viscosities, surface tension)
check_patch_physics()to validate initial condition constraints(positive pressure, valid volume fractions, positive dimensions)
check_time_stepping()to handle multiple time stepping modes(CFL-based, adaptive, fixed)
check_acoustic_source()with physics validation (positivefrequency, wavelength, gaussian parameters)
_is_numeric()helper and_format_errors()for better errorreporting with hints
definitions.py
Consolidated parameter definitions module with constraint anddependency supporttoolchain/mfc/params/definitions.py
compact module using loops instead of a definitions/ directory
dependencies (requires/recommends relationships)
_load()function that registers all parameters acrossCOMMON, PRE_PROCESS, SIMULATION, and POST_PROCESS stages
acoustic sources, probes, and other multi-instance configurations
user_guide.py
New user guide with enhanced help and interactive onboarding systemtoolchain/mfc/user_guide.py
with Rich formatting
setup, batch jobs, and debugging
failures, and run failures
interactive menu-driven interface
init.py
New case template generator with built-in and example-based templatestoolchain/mfc/init.py
built-in or example templates
1D_minimal,2D_minimal, and3D_minimalwith documented parameterscreate_case()function to generate new cases from templatesor copy from examples directory
list_templates()to display available templates andget_available_templates()for programmatic accessargs.py
Refactored argument parsing with centralized CLI schema and enhancedhelptoolchain/mfc/args.py
centralized CLI schema instead of manual argparse setup
print_help(),print_command_help(),and topic-based help system integration
_handle_enhanced_help()to provide rich formatting beforeargparse help
users
build.py
Enhanced build system with progress bars and improved error reportingtoolchain/mfc/build.py
output parsing using regex patterns
_run_build_with_progress()function supporting streamingmode (-v) and interactive progress bars
_show_build_error()to display detailed error information fromcaptured subprocess output
(✓/✗), timing information, and contextual error tips
-vv+ (full compiler output)
completion_gen.py
New shell completion generator for bash and zsh from CLI schematoolchain/mfc/cli/completion_gen.py
schema
generate_bash_completion()andgenerate_zsh_completion()functions for auto-generated completions
file types (Python, YAML, pack files)
argument sets in completion generation
argparse_gen.py
New argparse generator from declarative CLI schematoolchain/mfc/cli/argparse_gen.py
definitions
generate_parser()to convert schema into completeArgumentParser with subcommands and argument groups
dynamically from dataclass fields
exclusive groups, and nested subcommands
params_cmd.py
Parameter search and discovery CLI commandtoolchain/mfc/params_cmd.py
MFC case parameters
indexed parameters
configurable output formats
alternative suggestions
test.py
Enhanced test reporting and failure feedbacktoolchain/mfc/test/test.py
test names prominently
visual panel formatting
helpful hints
failed_teststracking and_print_test_summary()function fordetailed failure reporting
completion.py
Shell completion installation and managementtoolchain/mfc/completion.py
~/.local/share/mfc/completions/schema.py
CLI schema dataclass definitionstoolchain/mfc/cli/schema.py
truth)
Argument,Positional,Command,CommonArgumentSet, andCLISchemadataclassesArgActionandCompletionTypewith completionconfiguration
generation
7 files
case.py
Fix comment formatting in example caseexamples/2D_shearlayer/case.py
#'to# 'for consistency with Pythonstyle guidelines
case.py
Fix import statement formattingexamples/1D_bubblescreen/case.py
case.py
Code formatting improvement for import statementsexamples/nD_perfect_reactor/case.py
import json, argparseinto separatelines for PEP 8 compliance
case.py
Code formatting and style improvementsbenchmarks/5eq_rk3_weno3_hllc/case.py
##to single#for consistencycase.py
Comment style normalizationexamples/2D_axisym_shockwatercavity/case.py
##to single#for consistencycase.py
Comment style normalizationexamples/2D_ibm_steady_shock/case.py
##to single#for consistencyexport.py
Import statement formattingexamples/scaling/export.py
6 files
negative_tests.py
Negative test case generator for validator constraintstoolchain/mfc/params_tests/negative_tests.py
violate validator constraints
BASE_CASEwith valid parameters andConstraintTestdataclassfor test specification
generate_constraint_tests()covering simulation domain,model equations, time stepping, WENO, boundary conditions, and
acoustic source constraints
run_constraint_tests()andprint_test_report()for validationand reporting
mutation_tests.py
Mutation testing for validator coverage analysistoolchain/mfc/params_tests/mutation_tests.py
parameters to verify validator coverage
MUTATIONSdictionary with invalid values for ~40 parametersacross numeric, boolean, physics, and numerics categories
tracking
by parameter
snapshot.py
Validation snapshot tool for regression testingtoolchain/mfc/params_tests/snapshot.py
cases for regression testing
CaseSnapshotandValidationResultdataclasses for storingvalidation state
and save/load snapshots to JSON
across refactoring
coverage.py
Constraint coverage analysis tooltoolchain/mfc/params_tests/coverage.py
case_validator.pyAST
self.prohibit()calls and maps them to validation stagesrunner.py
Test safety net runner and orchestratortoolchain/mfc/params_tests/runner.py
test suite
build_safety_net()to capture parameter inventory,validation snapshots, and constraint coverage
verify_safety_net()to detect unintended validation behaviorchanges
generation commands
inventory.py
Parameter inventory export and analysistoolchain/mfc/params_tests/inventory.py
stages
export_parameter_inventory()categorizing parameters bystage and type
tracking
metadata
2 files
docs_gen.py
CLI documentation generator from schematoolchain/mfc/cli/docs_gen.py
definitions
examples, and subcommands
docs_gen.py
Parameter documentation generatortoolchain/mfc/params/generators/docs_gen.py
parameters
constraints
grouping for large families
instructions
74 files
CodeAnt-AI Description
Central registry for case parameters, comprehensive CLI schema, and interactive build/test UX
What Changed
Impact
✅ Clearer build errors and compiler output for faster debugging✅ Shorter time to find and understand case parameters✅ Shorter onboarding to create and run new cases via templates and shell completion💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
Documentation
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.