Improve registry type mismatch error by ansonnazeba · Pull Request #1390 · MFlowCode/MFC

ansonnazeba · 2026-05-01T00:35:20Z

Description

Improved the ParamRegistry type mismatch error message when the same parameter is registered with a conflicting type. Previously, the error only reported the parameter name. The new updated message now includes both the existing registered type and the new conflicting type, which makes the issue easier to diagnose.

Also updates the existing registry unit test to verify that the enhanced error details are included.

Fixes #N/A

Type of change

Testing

Tested the registry unit tests locally:

python3 -m unittest toolchain.mfc.params_tests.test_registry

qodo-code-review · 2026-05-01T00:35:27Z

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0)

1. Verbose enum repr in error 🐞 Bug ⚙ Maintainability

Description

ParamRegistry.register formats ParamType using !r, which for Enum produces verbose output like
"<ParamType.INT: 'int'>" instead of a stable, user-facing type string. This makes the improved
mismatch message harder to read and more likely to churn if formatting changes.

Code

toolchain/mfc/params/registry.py[R236-240]

+                raise ValueError(
+                    f"Type mismatch for '{param.name}': "
+                    f"existing type is {existing.param_type!r}, "
+                    f"new type is {param.param_type!r}"
+                )

Evidence

The new error message uses {existing.param_type!r}/{param.param_type!r}; ParamType is an Enum with
string values, so Enum repr will include both member and value (e.g., <ParamType.INT: 'int'>) rather
than the concise "int"/"real" values.

toolchain/mfc/params/registry.py[214-244]
toolchain/mfc/params/schema.py[14-23]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ParamRegistry.register()` uses `!r` when formatting `ParamType` in the type mismatch `ValueError`. For Enums this yields verbose strings like `<ParamType.INT: 'int'>`, which is noisy for a diagnostic message.
### Issue Context
`ParamType` is an `Enum` with string values (e.g. `INT = "int"`, `REAL = "real"`). For a clearer, more stable message, print `existing.param_type.value` / `param.param_type.value` (or `.name`) instead of `repr()`.
### Fix Focus Areas
- toolchain/mfc/params/registry.py[233-240]
- toolchain/mfc/params/schema.py[14-23]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. Mismatch test misses key details 🐞 Bug ⚙ Maintainability

Description

test_register_type_mismatch_raises only checks for generic substrings and does not verify that the
exception message contains the parameter name or the actual conflicting types. A regression could
remove these details while still passing the test.

Code

toolchain/mfc/params_tests/test_registry.py[R41-42]

+        self.assertIn("existing type is", str(ctx.exception))
+        self.assertIn("new type is", str(ctx.exception))

Evidence

The updated test asserts only "existing type is" and "new type is" and no longer asserts the param
name or which types were involved; meanwhile the implementation includes both details in the
message, so the test should lock those in.

toolchain/mfc/params_tests/test_registry.py[33-43]
toolchain/mfc/params/registry.py[233-240]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The unit test for type mismatch exceptions is too permissive: it only checks for generic substrings and does not verify that the message includes the parameter name and both conflicting types.
### Issue Context
The production code is intended to include the existing and new types in the error. The test should assert those details are present so future refactors don't accidentally remove them.
### Fix Focus Areas
- toolchain/mfc/params_tests/test_registry.py[33-43]
- toolchain/mfc/params/registry.py[233-240]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Previous review results

Review updated until commit 04ddb5e

Results up to commit 04ddb5e

🐞 Bugs (2) 📘 Rule violations (0)

1. Verbose enum repr in error 🐞 Bug ⚙ Maintainability

Description

ParamRegistry.register formats ParamType using !r, which for Enum produces verbose output like
"<ParamType.INT: 'int'>" instead of a stable, user-facing type string. This makes the improved
mismatch message harder to read and more likely to churn if formatting changes.

Code

toolchain/mfc/params/registry.py[R236-240]

+                raise ValueError(
+                    f"Type mismatch for '{param.name}': "
+                    f"existing type is {existing.param_type!r}, "
+                    f"new type is {param.param_type!r}"
+                )

Evidence

The new error message uses {existing.param_type!r}/{param.param_type!r}; ParamType is an Enum with
string values, so Enum repr will include both member and value (e.g., <ParamType.INT: 'int'>) rather
than the concise "int"/"real" values.

toolchain/mfc/params/registry.py[214-244]
toolchain/mfc/params/schema.py[14-23]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ParamRegistry.register()` uses `!r` when formatting `ParamType` in the type mismatch `ValueError`. For Enums this yields verbose strings like `<ParamType.INT: 'int'>`, which is noisy for a diagnostic message.

### Issue Context
`ParamType` is an `Enum` with string values (e.g. `INT = "int"`, `REAL = "real"`). For a clearer, more stable message, print `existing.param_type.value` / `param.param_type.value` (or `.name`) instead of `repr()`.

### Fix Focus Areas
- toolchain/mfc/params/registry.py[233-240]
- toolchain/mfc/params/schema.py[14-23]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. Mismatch test misses key details 🐞 Bug ⚙ Maintainability

Description

test_register_type_mismatch_raises only checks for generic substrings and does not verify that the
exception message contains the parameter name or the actual conflicting types. A regression could
remove these details while still passing the test.

Code

toolchain/mfc/params_tests/test_registry.py[R41-42]

+        self.assertIn("existing type is", str(ctx.exception))
+        self.assertIn("new type is", str(ctx.exception))

Evidence

The updated test asserts only "existing type is" and "new type is" and no longer asserts the param
name or which types were involved; meanwhile the implementation includes both details in the
message, so the test should lock those in.

toolchain/mfc/params_tests/test_registry.py[33-43]
toolchain/mfc/params/registry.py[233-240]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The unit test for type mismatch exceptions is too permissive: it only checks for generic substrings and does not verify that the message includes the parameter name and both conflicting types.

### Issue Context
The production code is intended to include the existing and new types in the error. The test should assert those details are present so future refactors don't accidentally remove them.

### Fix Focus Areas
- toolchain/mfc/params_tests/test_registry.py[33-43]
- toolchain/mfc/params/registry.py[233-240]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

qodo-code-review · 2026-05-01T00:37:01Z

Persistent review updated to latest commit 04ddb5e

coderabbitai · 2026-05-01T00:37:56Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f3c1b221-be1d-4650-9b2e-edffa8e67b4a

📥 Commits

Reviewing files that changed from the base of the PR and between a314b0b and 04ddb5e.

📒 Files selected for processing (2)

toolchain/mfc/params/registry.py
toolchain/mfc/params_tests/test_registry.py

📝 Walkthrough

Walkthrough

The pull request modifies error handling in the parameter registry. The ParamRegistry.register method now provides more detailed error messages when a parameter name already exists with a different type. The error message is updated to explicitly include both the existing parameter type and the new type being registered, using repr formatting. A corresponding test is updated to validate the new error message format.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: improving the error message for registry type mismatches, which directly corresponds to the changeset.
Description check	✅ Passed	The description covers the key aspects of the change and includes the required type of change, testing details, and necessary context about the improvements.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Review rate limit: 0/1 reviews remaining, refill in 60 minutes.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

codecov · 2026-05-01T02:42:58Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.75%. Comparing base (00aa603) to head (04ddb5e).
⚠️ Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1390      +/-   ##
==========================================
- Coverage   64.76%   64.75%   -0.01%     
==========================================
  Files          71       71              
  Lines       18713    18716       +3     
  Branches     1549     1548       -1     
==========================================
+ Hits        12119    12120       +1     
- Misses       5638     5641       +3     
+ Partials      956      955       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Improve registry type mismatch error

04ddb5e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve registry type mismatch error#1390

Improve registry type mismatch error#1390
ansonnazeba wants to merge 1 commit intoMFlowCode:masterfrom
ansonnazeba:improve-python-validation-message

ansonnazeba commented May 1, 2026

Uh oh!

qodo-code-review Bot commented May 1, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot commented May 1, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented May 1, 2026

Walkthrough

Uh oh!

codecov Bot commented May 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

1 participant

Conversation

ansonnazeba commented May 1, 2026

Description

Type of change

Testing

Uh oh!

qodo-code-review Bot commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Previous review results

Uh oh!

qodo-code-review Bot commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented May 1, 2026

Walkthrough

Uh oh!

codecov Bot commented May 1, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

1 participant

qodo-code-review Bot commented May 1, 2026 •

edited

Loading

qodo-code-review Bot commented May 1, 2026 •

edited

Loading