Remove dead SIM108 ignore for middle_earth; showcase ternaries in mettsim#129
Merged
Conversation
The vectorizer supports ternary expressions (IfExp -> xnp.where), and middle_earth has no SIM108 violations, so the per-file ignore is dead config. Keep the ignore for tests/tt/test_vectorization.py, whose statement-style if/else blocks are intentional fixtures, and say so in the comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
hmgaudecker
approved these changes
Jun 6, 2026
| "PLR1716", # Boolean chained comparison | ||
| "RET", # Unnecessary return logic | ||
| "SIM108", # Use ternary operator | ||
| "SIM108", # Statement-style if/else blocks are intentional fixtures for the vectorizer |
Contributor
There was a problem hiding this comment.
Fine, but we should add a test for the new behaviour, or is it already in place?
I'm amazed we don't have a single instance of an if/else block in METTSIM.
Member
Author
There was a problem hiding this comment.
We have if/else blocks, the SIM108 rule is about how they're written up (and we already wrote everything in a compliant way).
I added two oneline if/else conditions to the code and tightened the RET hook (we can allow all codes but RET505 I believe).
AFAICT there is no rule that disallowed these oneliners. Sorry, should have made that clear earlier.
Showcase that the vectorizer handles ternary expressions (ast.IfExp -> xnp.where) in real policy functions, not just in the unit-test fixtures (f2/f6/f7/f10 in tests/tt/test_vectorization.py). The converted functions are swept by test_convertible for all policy years. Keep statement-style if/else where it is multi-branch (payroll_tax amount_y), over the line limit (orc bounty), an integration-test subject (housing_benefits amount_m_fam), or an intentional fixture (annotation_bug_reproducer). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
MImmesberger
changed the title
mettsim doubles as test material for the vectorizer, so it should exercise both supported styles. Revert the housing-benefit income and property-tax cap conversions to block form; keep the ternaries only where they are genuine short one-liners (wealth tax, child tax credit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Only RET505 (superfluous else after return) conflicts with the vectorizer: its autofix produces the early-return form that _if_to_call rejects (g3 fixture). The rest of the RET family passes clean on mettsim and its autofixes are vectorizer-safe, so enforce it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hmgaudecker
reviewed
Jun 7, 2026
| @@ -240,7 +239,7 @@ per-file-ignores."tests/tt/test_vectorization.py" = [ | |||
| "PLR1714", # Repeated equality comparison | |||
| "PLR1716", # Boolean chained comparison | |||
| "RET", # Unnecessary return logic | |||
Contributor
There was a problem hiding this comment.
Suggested change
| "RET", # Unnecessary return logic | |
| "RET505", # Vectorizer requires both if/else branches; it rejects the early-return form |
?
Contributor
There was a problem hiding this comment.
I wonder whether we need all that duplication in the first place, though.
Contributor
|
😇 deleted a message where my own bad memory fooled me, sorry for the email noise, @MImmesberger |
The E721/PLR1714/PLR1716 ignores were inherited from gettsim's config in the initial ttsim split and never fire in either file. The blanket RET ignore for test_vectorization.py covers exactly two live rules: RET504 (the `out = ...; return out` fixtures mirror the vectorizer's output) and RET505 (return-style if/else fixtures). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
MImmesberger
added a commit
to ttsim-dev/gettsim
that referenced
this pull request
Jun 9, 2026
Following ttsim-dev/ttsim#129: only RET505 (superfluous-else-after-return) conflicts with the vectorizer, whose `_if_to_call` rejects the early-return form RET505's autofix produces. The rest of the RET family is vectorizer-safe, so enforce it. - pyproject.toml: replace blanket "RET" ignore for src/gettsim/germany/* with "RET505". Clarify PLR1714/PLR1716 comments ("not vectorizable") — unlike mettsim these are real, needed ignores: vectorization.py has no visit_Compare, so `in` and chained comparisons are not lowered to element-wise ops. - Fix the 5 now-enforced RET504 sites (out = ...; return out -> direct return): vorsorge.py, ids.py (x3), erwerbsminderung.py. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Three related changes, following the discovery in ttsim-dev/gettsim#1178 (comment) that the vectorizer handles ternary expressions (
ast.IfExp→xnp.wherevia_ifexp_to_call):Remove the dead SIM108 ignore for
src_mettsim/mettsim/middle_earth/**/*.py. The ignore never suppressed anything: SIM108 only flags assignment-style if/else (if c: x = a / else: x = b; verified on ruff 0.13.0, stable and preview alike), while mettsim's if/else blocks are return-style — which no ruff rule converts to a ternary. The single assignment-style block (orc bounty) exceeds the line limit, where SIM108 auto-skips.Convert two return-style if/else to ternaries (wealth tax, child tax credit) so that mettsim showcases ternaries in real policy functions — these are swept by
test_convertiblefor all policy years. Only sites where the ternary is a genuine short one-liner are converted: mettsim doubles as test material for the vectorizer, so it deliberately keeps exercising both supported styles. Vectorization of ternaries was already unit-tested viaf2/f6/f7/f10intests/tt/test_vectorization.py.Narrow mettsim's blanket
RETignore toRET505. Only RET505 (superfluous else after return) conflicts with the vectorizer: its autofix produces the early-return form (if c: return a+ barereturn b) that_if_to_callrejects by design (see theg3fixture). The rest of the RET family passes clean on mettsim and its autofixes are vectorizer-safe, so it is now enforced.The SIM108 ignore for
tests/tt/test_vectorization.pystays: its statement-style if/else functions are intentional fixtures for the GAST converter.Companion sweep for gettsim's policy code: ttsim-dev/gettsim#1195.
🤖 Generated with Claude Code