07 13 chore lint this project to an inch of its life - #51
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @fsargent, your pull request is larger than the review limit of 150000 diff characters
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (22)
📝 WalkthroughWalkthroughThe PR updates linting and project configuration, modernizes Python imports and argument handling, fixes voting-method behavior, adds deterministic IRV simulation and chart-generation scripts, expands doctest registration, and refreshes published VSE documentation and assets. ChangesSimulation and voting updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant recalculate
participant ProcessPoolExecutor
participant PageAssets
CLI->>recalculate: elections, seed, workers
recalculate->>ProcessPoolExecutor: deterministic chunk jobs
ProcessPoolExecutor-->>recalculate: VSE and scenario aggregates
recalculate->>PageAssets: refreshed HTML and PNG chart assets
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
ec63879 to
01547ed
Compare
fb64750 to
755e75a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mydecorators.py (1)
166-166: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
curriedstill uses Python 2func_code—mydecorators.py:166will raiseAttributeErrorunder Python 3.10+. Replace it withself.func.__code__.co_argcount.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mydecorators.py` at line 166, Update the argument-count check in curried to use the Python 3 code object attribute self.func.__code__.co_argcount instead of the obsolete self.func.func_code.co_argcount, preserving the existing comparison and currying behavior.
🧹 Nitpick comments (7)
sodaTest.py (2)
11-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: unused
_varkwand fragilekwargs['exclude']access.
_varkwis unpacked but never used (F841). Consider using_*or removing it. Additionally,kwargs['exclude']on line 14 willKeyErrorifkwargsis non-empty but lacks an'exclude'key — usingkwargs.get('exclude', [])would be more robust.♻️ Proposed refactor
- attrs,varargs,_varkw,defaults = spec.args, spec.varargs, spec.varkw, spec.defaults + attrs, varargs, _varkw, defaults = spec.args, spec.varargs, spec.varkw, spec.defaults- if kwargs and attr in kwargs['exclude']: + if kwargs and attr in kwargs.get('exclude', []):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sodaTest.py` around lines 11 - 12, Update the argument unpacking near inspect.getfullargspec(func) to avoid binding the unused _varkw value, and change the kwargs['exclude'] access in the surrounding function to use a default empty list when the key is absent.Source: Linters/SAST tools
75-85: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMutable default
cantWin=[]is a latent shared-state bug.The mutable default
cantWin = []is shared across all calls that don't passcantWin. SincecantWinis later mutated via.add()(line 282), state can leak betweenElectionCountsinstances. Ruff flags this as B006. While this appears to be a pre-existing pattern (also on lines 128 and 190), this PR's lint focus makes it worth addressing.♻️ Proposed fix
- def __init__(self, delg, appr, prefs, order, cantWin = [], oldSmith = None): + def __init__(self, delg, appr, prefs, order, cantWin = None, oldSmith = None): """ delg: A list of n delegation counts appr: A list of n approval counts prefs: A list of n preference lists counts order: delegation order. """ + if cantWin is None: + cantWin = [] self.n = len(delg)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sodaTest.py` around lines 75 - 85, Update the ElectionCounts.__init__ parameter cantWin to avoid a mutable list default, using a None sentinel and initializing a fresh list inside the constructor when no value is provided. Preserve caller-supplied cantWin values and apply the same fix to the other mutable default parameters in the nearby constructors identified by the review.Source: Linters/SAST tools
voterModels.py (1)
1-1: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefer
strict=Trueoverstrict=Falsefor these equal-lengthzipcalls.Both sites were updated only to satisfy Ruff's B905 (
zip-without-explicit-strict), but both chose the lenient option.Electorate.socUtilsassumes every voter has the same number of candidate utilities, andDimVoter.fromDimsassumesv,c,e.dimWeights, andcaringall sharendims. Withstrict=False, a length mismatch (e.g. a malformed electorate) would silently truncate and produce a wrong-but-plausible social-utility/candidate-distance result instead of failing loudly.
voterModels.py#L125: changezip(*self, strict=False)tozip(*self, strict=True)inElectorate.socUtils.voterModels.py#L235-243: change bothzip(caring, e.dimWeights, strict=False)andzip(v,c,e.dimWeights,caring, strict=False)tostrict=TrueinDimVoter.fromDims.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@voterModels.py` at line 1, Update the zip calls in Electorate.socUtils and DimVoter.fromDims to use strict=True instead of strict=False, preserving loud failure on mismatched voter, candidate, dimension-weight, or caring-vector lengths..trunk/configs/ruff.toml (1)
2-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
B006enabledThere are several mutable-default sites in the repo (
methods.py:getLeast,dataClasses.py:CandidateWithCount.__init__,sodaTest.py:ElectionCounts.__init__,sodaTest.py:beaters,sodaTest.py:growFrom).B006is still catching a real bug class here, so remove it fromignoreonly after these are cleaned up;B008/E501can stay ignored.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.trunk/configs/ruff.toml around lines 2 - 5, The Ruff configuration currently ignores B006 despite mutable-default issues remaining in methods.py:getLeast, dataClasses.py:CandidateWithCount.__init__, sodaTest.py:ElectionCounts.__init__, sodaTest.py:beaters, and sodaTest.py:growFrom. Clean up those mutable defaults using appropriate per-call initialization, then remove B006 from the ignore list while keeping B008 and E501 ignored.pyproject.toml (1)
12-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
--doctest-modulesandload_testsintests.pywill collect the same doctests twice.
addopts = "--doctest-modules"makes pytest scan all Python files for doctests.tests.py:14-22also addsdoctest.DocTestSuiteforvse,voterModels,stratFunctions,methods, anddataClassesvia theload_testsprotocol. The five modules' doctests will execute twice. Ifload_testsis only forunittest-without-pytest compatibility, consider guarding it with apytestcheck or removing it entirely in favor of--doctest-modules.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` around lines 12 - 13, Remove the duplicate doctest collection by updating the tests.py load_tests integration for vse, voterModels, stratFunctions, methods, and dataClasses, or guard it when running under pytest; retain --doctest-modules as the sole pytest collection path while preserving unittest compatibility if required..trunk/trunk.yaml (1)
37-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the Trunk Python runtime with the supported interpreter range
.trunk/trunk.yaml:37pinspython@3.14.4, whilepyproject.tomldeclaresrequires-python = ">=3.10,<3.13". Use a Python runtime within the supported range (for example,python@3.12.x) so Python-based lint tooling stays aligned with the project matrix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.trunk/trunk.yaml at line 37, Update the Python runtime entry in Trunk’s configuration from python@3.14.4 to a version within the project’s declared >=3.10,<3.13 range, such as python@3.12.x, keeping the Python lint tooling aligned with the supported interpreter matrix.scripts/regenerate_pages_images.py (1)
183-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUseless ternary — both branches yield the same value.
left=0.14 if selected else 0.14always evaluates to0.14; simplify.♻️ Proposed fix
- figure.subplots_adjust(left=0.14 if selected else 0.14, right=0.80, bottom=0.12, top=0.96) + figure.subplots_adjust(left=0.14, right=0.80, bottom=0.12, top=0.96)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/regenerate_pages_images.py` at line 183, In the figure.subplots_adjust call, simplify the left argument by removing the redundant conditional expression and use the constant 0.14 directly; leave the other layout parameters unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dataClasses.py`:
- Around line 64-68: Update fullSerialize so its try block performs the intended
serialization operation instead of pass, ensuring the method returns the
serialized values and preserves the AttributeError fallback that builds a list
from self.keys(). Verify the Tallies behavior continues to produce [[5], [4],
[3], []].
- Line 4: Remove the test-only import of isnum from dataClasses.py and provide
the helper locally or through an appropriate shared utility module. Update all
isnum usages in dataClasses.py to reference the relocated definition, without
changing its behavior.
In `@docs/vsebreakdown.html`:
- Line 1: Remove the stray “Star” text before the <!doctype html> declaration in
docs/vsebreakdown.html, ensuring the doctype is the first document content so
standards-mode rendering is preserved.
In `@methods.py`:
- Around line 2-8: Update the NumPy imports in methods.py by replacing the
invalid numpy.core.fromqnumeric mean import with mean imported directly from
numpy; leave the other imports unchanged.
- Around line 273-275: Remove the trailing periods from the doctest output
examples associated with Srv().results and the output at the second documented
location, leaving the list values unchanged so doctest comparisons match the
actual returned lists.
- Around line 788-792: Remove the `assert o1s[1] < o1s[2]` statement in the
semifinalist scoring logic, preserving the surrounding
`r2s[semifinalists[o1s[0]]]` adjustment and allowing valid doctest ballots such
as `n1s=[2,1,1]` to proceed without enforcing this invalid ordering invariant.
In `@scripts/regenerate_pages_images.py`:
- Around line 8-20: Move the scripts.recalculate_irv_pages import, including
DEFAULT_WORKERS and recalculate, below the ROOT calculation and sys.path.insert
bootstrap in regenerate_pages_images.py so direct script execution resolves the
module correctly.
In `@sodaTest.py`:
- Around line 316-323: Update the score adjustment in the scores method’s final
loop so the approval value is multiplied by self.n - 1 as a grouped expression,
preserving the existing accumulation behavior for each j.
In `@stratFunctions.py`:
- Line 64: Update the chooser initialization around __init__ so a zero-argument
OssChooser or LazyChooser receives a valid default subChoosers collection before
any dereference. Preserve explicitly provided subChoosers and ensure the
no-argument paths used by dataClasses and vse remain runtime-safe.
In `@voterModels.py`:
- Around line 267-270: Remove the trailing period from the expected output in
the DimModel doctest for dm.dimWeights, leaving the assertion as the list
representation [1, 0.5] so it matches Python repr output.
- Around line 2-8: Update the import block in voterModels.py to import std from
numpy alongside mean, preserving the existing public name used by the module’s
doctests.
---
Outside diff comments:
In `@mydecorators.py`:
- Line 166: Update the argument-count check in curried to use the Python 3 code
object attribute self.func.__code__.co_argcount instead of the obsolete
self.func.func_code.co_argcount, preserving the existing comparison and currying
behavior.
---
Nitpick comments:
In @.trunk/configs/ruff.toml:
- Around line 2-5: The Ruff configuration currently ignores B006 despite
mutable-default issues remaining in methods.py:getLeast,
dataClasses.py:CandidateWithCount.__init__, sodaTest.py:ElectionCounts.__init__,
sodaTest.py:beaters, and sodaTest.py:growFrom. Clean up those mutable defaults
using appropriate per-call initialization, then remove B006 from the ignore list
while keeping B008 and E501 ignored.
In @.trunk/trunk.yaml:
- Line 37: Update the Python runtime entry in Trunk’s configuration from
python@3.14.4 to a version within the project’s declared >=3.10,<3.13 range,
such as python@3.12.x, keeping the Python lint tooling aligned with the
supported interpreter matrix.
In `@pyproject.toml`:
- Around line 12-13: Remove the duplicate doctest collection by updating the
tests.py load_tests integration for vse, voterModels, stratFunctions, methods,
and dataClasses, or guard it when running under pytest; retain --doctest-modules
as the sole pytest collection path while preserving unittest compatibility if
required.
In `@scripts/regenerate_pages_images.py`:
- Line 183: In the figure.subplots_adjust call, simplify the left argument by
removing the redundant conditional expression and use the constant 0.14
directly; leave the other layout parameters unchanged.
In `@sodaTest.py`:
- Around line 11-12: Update the argument unpacking near
inspect.getfullargspec(func) to avoid binding the unused _varkw value, and
change the kwargs['exclude'] access in the surrounding function to use a default
empty list when the key is absent.
- Around line 75-85: Update the ElectionCounts.__init__ parameter cantWin to
avoid a mutable list default, using a None sentinel and initializing a fresh
list inside the constructor when no value is provided. Preserve caller-supplied
cantWin values and apply the same fix to the other mutable default parameters in
the nearby constructors identified by the review.
In `@voterModels.py`:
- Line 1: Update the zip calls in Electorate.socUtils and DimVoter.fromDims to
use strict=True instead of strict=False, preserving loud failure on mismatched
voter, candidate, dimension-weight, or caring-vector lengths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c36090d-2d30-4457-8be7-c578e7b5282e
⛔ Files ignored due to path filters (5)
docs/5vse.pngis excluded by!**/*.pngdocs/5vse_small.pngis excluded by!**/*.pngdocs/vse.pngis excluded by!**/*.pngdocs/vsestrat.pngis excluded by!**/*.pnguv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.trunk/configs/.yamllint.yaml.trunk/configs/ruff.toml.trunk/trunk.yamldataClasses.pydocs/404.htmldocs/VSEbasic.mddocs/index.mddocs/irv-simulation.mddocs/stratbreakdown.htmldocs/stratstuff.htmldocs/vse-graph.htmldocs/vsebreakdown.htmlmethods.pymydecorators.pypyproject.tomlscripts/recalculate_irv_pages.pyscripts/regenerate_pages_images.pysodaTest.pystratFunctions.pytests.pyvoterModels.pyvse.py
| @@ -1,47 +1,2123 @@ | |||
| Star<!DOCTYPE html> | |||
| Star<!doctype html> | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the stray Star prefix before the doctype.
Star becomes visible document content and causes the doctype to be ignored, potentially triggering quirks-mode rendering.
Proposed fix
-Star<!doctype html>
+<!doctype html>📝 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.
| Star<!doctype html> | |
| <!doctype html> |
🧰 Tools
🪛 HTMLHint (1.9.2)
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/vsebreakdown.html` at line 1, Remove the stray “Star” text before the
<!doctype html> declaration in docs/vsebreakdown.html, ensuring the doctype is
the first document content so standards-mode rendering is preserved.
Source: Linters/SAST tools
| >>> Srv().results([[0,4,3,1,2]] * 5 + [[1,4,3,2,1]] * 4 + [[2,3,4,0,1]] * 6) | ||
| [1.0666666666666667, 3.6, 3.4, 0.8666666666666667, 1.3333333333333333] | ||
| [1.0666666666666667, 3.6, 3.4, 0.8666666666666667, 1.3333333333333333]. | ||
| """ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l methods.py
echo '--- lines 260-285 ---'
sed -n '260,285p' methods.py
echo '--- lines 620-640 ---'
sed -n '620,640p' methods.py
echo '--- doctest references ---'
rg -n "doctest|DocTest|__test__|pytest\.doctest|collect" methods.pyRepository: electionscience/vse-sim
Length of output: 2016
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('methods.py')
text = p.read_text()
for needle in [
"Srv().results([[0,4,3,1,2]] * 5 + [[1,4,3,2,1]] * 4 + [[2,3,4,0,1]] * 6)",
"[1.0666666666666667, 3.6, 3.4, 0.8666666666666667, 1.3333333333333333].",
"[1, 2, 3, 0].",
]:
print(needle, '=>', needle in text)
PYRepository: electionscience/vse-sim
Length of output: 344
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden --glob '!.git' --glob '!node_modules' --glob '!dist' --glob '!build' \
"doctest|pytest\.mark\.doctest|--doctest|addopts|python -m doctest|DocTestFinder|DocFileSuite" .Repository: electionscience/vse-sim
Length of output: 995
Remove the trailing periods from these doctest outputs
methods.py:273-275:[1.0666666666666667, 3.6, 3.4, 0.8666666666666667, 1.3333333333333333].→[1.0666666666666667, 3.6, 3.4, 0.8666666666666667, 1.3333333333333333]methods.py:632-633:[1, 2, 3, 0].→[1, 2, 3, 0]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@methods.py` around lines 273 - 275, Remove the trailing periods from the
doctest output examples associated with Srv().results and the output at the
second documented location, leaving the list values unchanged so doctest
comparisons match the actual returned lists.
557d975 to
bc6ba2d
Compare
bc6ba2d to
dab8178
Compare
dab8178 to
80475fd
Compare
1 participant
Summary
Verification
trunk fmt --ci --no-progress --alltrunk check --ci --no-progress --all --filter=rufftrunk check --ci --no-progress --all --filter=markdownlintSummary by CodeRabbit
New Features
Bug Fixes
Documentation