feat: add CI/CD enhancements, test infrastructure, and tooling#255
Open
you615074-png wants to merge 1 commit into
Open
feat: add CI/CD enhancements, test infrastructure, and tooling#255you615074-png wants to merge 1 commit into
you615074-png wants to merge 1 commit into
Conversation
- Add pyproject.toml with ruff, mypy, pytest configuration - Add .pre-commit-config.yaml with linting and formatting hooks - Add Makefile with 12 targets for common project commands - Enhance CI workflow with 4 new jobs: python-syntax, typescript-syntax, quiz-coverage, and pytest matrix - Add scripts/quiz_coverage.py for per-phase quiz coverage reporting - Add scripts/scaffold_quizzes.py to generate quiz.json skeletons for 165+ lessons currently missing quizzes - Add scripts/scaffold_tests.py to generate pytest skeletons for lessons without tests (~470 lessons) - Add scripts/ts_syntax_check.mjs for TypeScript syntax checking without dependency resolution (handles hono/zod/anthropic imports gracefully) - Fix LESSON_TEMPLATE.md to match actual lesson structure: remove notebook/ directory, add quiz.json schema, add agent-*.md outputs, revise code comment guidelines - Add 45 concrete example tests: - Agent Loop (19 tests): ToyLLM, ToolRegistry, Calculator, KVStore, AgentLoop - Linear Algebra (26 tests): Vector, Matrix, is_independent, Gram-Schmidt Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR establishes comprehensive developer infrastructure for a curriculum project: it defines lesson structure standards, configures linting and testing tooling, introduces automation scripts for quiz and test scaffolding, unifies developer commands in a Makefile, and integrates advisory checks into CI workflows. ChangesCurriculum Infrastructure & Developer Tooling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
phases/01-math-foundations/01-linear-algebra-intuition/code/tests/test_main.py (1)
23-26: 💤 Low valueAdd
strict=Truetozip()for safer iteration.Line 25 uses
zip()without an explicitstrict=parameter. Since the test file requires Python 3.10+, you can addstrict=Trueto ensure the component lists have equal length (though they should by construction).♻️ Proposed change
def test_sub(self) -> None: v = vectors.Vector([5, 7, 9]) - vectors.Vector([1, 2, 3]) - for a, b in zip(v.components, [4.0, 5.0, 6.0]): + for a, b in zip(v.components, [4.0, 5.0, 6.0], strict=True): assert abs(a - b) < 1e-10🤖 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 `@phases/01-math-foundations/01-linear-algebra-intuition/code/tests/test_main.py` around lines 23 - 26, Update the zip invocation in the test_sub test to use strict=True to ensure both sequences are the same length; in function test_sub (which constructs v = vectors.Vector([5, 7, 9]) - vectors.Vector([1, 2, 3]) and iterates over v.components), change the zip(v.components, [4.0, 5.0, 6.0]) call to zip(v.components, [4.0, 5.0, 6.0], strict=True) so mismatched lengths raise an error rather than silently truncating.scripts/ts_syntax_check.mjs (1)
90-94: 💤 Low valueSimplify diagnostics assignment (optional).
Lines 91-93 use
[].concat()to wrap the result ofgetParseDiagnostics(), butgetParseDiagnostics()already returns an array. You can simplify to:const diagnostics = ts.getParseDiagnostics(sf);🤖 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/ts_syntax_check.mjs` around lines 90 - 94, The diagnostics assignment currently wraps ts.getParseDiagnostics(sf) in [].concat() unnecessarily; update the diagnostics variable to directly use the array returned by ts.getParseDiagnostics(sf) (replace the [].concat(ts.getParseDiagnostics(sf)) expression with ts.getParseDiagnostics(sf)) so the code reads const diagnostics = ts.getParseDiagnostics(sf); and remove the redundant concat wrapper.LESSON_TEMPLATE.md (1)
165-177: ⚡ Quick winConsider adding the 5+ unit tests requirement.
The "Code File Guidelines" section documents header comments and inline comment policy, but doesn't mention the 5+ unit tests minimum that is enforced for lesson code. Based on learnings, lesson code must include at least 5 unit tests runnable via stdlib test runner.
📋 Suggested addition
## Code File Guidelines - Code must run without errors - Add a 4–6 line header comment citing the lesson path and any external spec/RFC referenced by the implementation - Use inline comments sparingly — let the code speak, but don't be dogmatic about "zero comments." Some algorithms need a one-liner to orient the reader - Use the language that fits best for the topic - Include a `# requires: pkg1, pkg2` comment at the top if your entry file needs packages outside the Python stdlib (see `scripts/lesson_run.py`) +- Include 5+ unit tests in `code/tests/`, runnable via the stdlib test runner + (`python3 -m unittest discover`, `npx tsx --test`, Rust/Julia inline) - Start simple, build up complexity - Every function and class should have a clear purposeBased on learnings, lesson code must include 5+ unit tests minimum, runnable via stdlib test runner.
🤖 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 `@LESSON_TEMPLATE.md` around lines 165 - 177, Update the "Code File Guidelines" section to explicitly require "Include at least 5 unit tests" that are runnable via the Python stdlib test runner; specifically add a bullet saying lesson code must include 5+ unit tests in files named test_*.py (or discoverable by python -m unittest) and that tests should run with the project's runner (scripts/lesson_run.py) and not rely on external frameworks unless declared in a `# requires:` header. This clarifies the enforced minimum and how tests must be structured and executed.
🤖 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 @.github/workflows/curriculum.yml:
- Around line 81-94: The typescript-syntax job is labeled advisory but will fail
the workflow because the step running node scripts/ts_syntax_check.mjs exits
non-zero on errors; update the typescript-syntax job to be non-blocking by
either (A) replacing the single-step run of node scripts/ts_syntax_check.mjs in
the "syntax-check all .ts files" step with a small wrapper that captures the
JSON output from scripts/ts_syntax_check.mjs, parses it and prints warnings
(mirroring the python-syntax handling), and exits 0, or (B) simpler: set
continue-on-error: true on the "syntax-check all .ts files" step so failures are
advisory; modify the typescript-syntax job/step accordingly.
In @.pre-commit-config.yaml:
- Around line 31-38: The comment "mypy is advisory only — do not block commits"
is misleading because the pre-commit hook with id: mypy currently blocks commits
on failure; either remove that comment or make the hook non-blocking by adding
the advisory option (e.g., add verbose: true or set the hook to not fail) to the
mypy hook configuration; update the block that contains repo:
https://github.com/pre-commit/mirrors-mypy, rev: v1.15.0, hooks -> - id: mypy to
reflect the chosen change.
In `@Makefile`:
- Around line 41-42: The Makefile test target uses "--timeout=60" which
conflicts with pyproject.toml's "timeout = 30"; update the Makefile test target
(the "test:" rule invoking $(PYTHON) -m pytest --timeout=60 -x) to use the same
timeout value as pyproject.toml (30) or, if 60s is intentional, add a brief
comment above the "test:" target explaining why this override is required so
maintainers understand the discrepancy.
- Around line 70-72: The scaffold Makefile target currently only validates SLUG;
add validation for PHASE as well so both required variables are checked before
calling scaffold-lesson.sh: in the scaffold target (symbol: scaffold) ensure you
test -n "$(PHASE)" and test -n "$(SLUG)" and emit the usage message "Usage: make
scaffold PHASE=NN SLUG=lesson-name" and exit non‑zero if either is missing, then
invoke bash scripts/scaffold-lesson.sh $(PHASE) $(SLUG) only when both are
present.
In `@scripts/quiz_coverage.py`:
- Around line 148-152: The printed phase path reconstruction is lossy—stop
rebuilding the slug from c.phase_name; instead preserve and use the original
directory slug in PhaseCoverage. Update the code that builds PhaseCoverage
(e.g., constructor/factory for PhaseCoverage and the function that maps slugs to
names like _phase_slug_to_name) to store the original slug into a field (e.g.,
phase_slug or original_slug), and then change the printing loop (the block
iterating coverages and using c.phase_id and c.phase_name) to use that stored
slug (c.phase_slug) when formatting the output path so the exact original
directory name is emitted.
In `@scripts/scaffold_tests.py`:
- Around line 83-87: In the lines that call lines.append which build the comment
block (the three calls that currently use f-strings but contain no
placeholders), remove the unnecessary leading "f" so they become normal string
literals (keep the final lines.append that uses fname/rel/py_files as an
f-string since it contains placeholders); look for the lines.append(...) calls
that reference rel, fname and py_files to identify the correct statements and
change only the f-prefixed literals that have no interpolation.
---
Nitpick comments:
In `@LESSON_TEMPLATE.md`:
- Around line 165-177: Update the "Code File Guidelines" section to explicitly
require "Include at least 5 unit tests" that are runnable via the Python stdlib
test runner; specifically add a bullet saying lesson code must include 5+ unit
tests in files named test_*.py (or discoverable by python -m unittest) and that
tests should run with the project's runner (scripts/lesson_run.py) and not rely
on external frameworks unless declared in a `# requires:` header. This clarifies
the enforced minimum and how tests must be structured and executed.
In
`@phases/01-math-foundations/01-linear-algebra-intuition/code/tests/test_main.py`:
- Around line 23-26: Update the zip invocation in the test_sub test to use
strict=True to ensure both sequences are the same length; in function test_sub
(which constructs v = vectors.Vector([5, 7, 9]) - vectors.Vector([1, 2, 3]) and
iterates over v.components), change the zip(v.components, [4.0, 5.0, 6.0]) call
to zip(v.components, [4.0, 5.0, 6.0], strict=True) so mismatched lengths raise
an error rather than silently truncating.
In `@scripts/ts_syntax_check.mjs`:
- Around line 90-94: The diagnostics assignment currently wraps
ts.getParseDiagnostics(sf) in [].concat() unnecessarily; update the diagnostics
variable to directly use the array returned by ts.getParseDiagnostics(sf)
(replace the [].concat(ts.getParseDiagnostics(sf)) expression with
ts.getParseDiagnostics(sf)) so the code reads const diagnostics =
ts.getParseDiagnostics(sf); and remove the redundant concat wrapper.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b2507312-4dd0-4ebb-a218-321db4cb3916
📒 Files selected for processing (11)
.github/workflows/curriculum.yml.pre-commit-config.yamlLESSON_TEMPLATE.mdMakefilephases/01-math-foundations/01-linear-algebra-intuition/code/tests/test_main.pyphases/14-agent-engineering/01-the-agent-loop/code/tests/test_main.pypyproject.tomlscripts/quiz_coverage.pyscripts/scaffold_quizzes.pyscripts/scaffold_tests.pyscripts/ts_syntax_check.mjs
Comment on lines
+81
to
+94
| typescript-syntax: | ||
| name: typescript syntax check (advisory) | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 | ||
| with: | ||
| persist-credentials: false | ||
| - uses: actions/setup-node@e62d1c64a114b2d4b223e6e62319cbfa45a42c6a # v4 | ||
| with: | ||
| node-version: "22" | ||
| - name: install typescript | ||
| run: npm install --no-save typescript | ||
| - name: syntax-check all .ts files | ||
| run: node scripts/ts_syntax_check.mjs --json |
There was a problem hiding this comment.
TypeScript syntax job blocks on failure despite "advisory" label.
The job name includes "(advisory)" but the step will fail and block the workflow if ts_syntax_check.mjs exits with code 1. From the TypeScript syntax checker implementation, it returns exit code 1 when syntax errors are found.
For consistency with the python-syntax job (lines 58-79), either add error handling to parse the JSON output and emit warnings without failing, or use continue-on-error: true to make this truly advisory.
🔧 Proposed fix options
Option 1: Parse JSON and emit warnings like python-syntax
- name: syntax-check all .ts files
- run: node scripts/ts_syntax_check.mjs --json
+ run: |
+ node scripts/ts_syntax_check.mjs --json > /tmp/ts-syntax.json
+ node -e "
+ const data = require('/tmp/ts-syntax.json');
+ const failed = data.results.filter(r => r.status === 'failed');
+ console.log(\`TypeScript: \${data.checked} files, \${data.passed} passed, \${data.failed} failed\`);
+ for (const f of failed) {
+ console.log(\`::warning file=\${f.file}::syntax error: \${f.message}\`);
+ }
+ "Option 2: Use continue-on-error
- name: syntax-check all .ts files
run: node scripts/ts_syntax_check.mjs --json
+ continue-on-error: true📝 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
| typescript-syntax: | |
| name: typescript syntax check (advisory) | |
| runs-on: ubuntu-latest | |
| steps: | |
| - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 | |
| with: | |
| persist-credentials: false | |
| - uses: actions/setup-node@e62d1c64a114b2d4b223e6e62319cbfa45a42c6a # v4 | |
| with: | |
| node-version: "22" | |
| - name: install typescript | |
| run: npm install --no-save typescript | |
| - name: syntax-check all .ts files | |
| run: node scripts/ts_syntax_check.mjs --json | |
| typescript-syntax: | |
| name: typescript syntax check (advisory) | |
| runs-on: ubuntu-latest | |
| steps: | |
| - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 | |
| with: | |
| persist-credentials: false | |
| - uses: actions/setup-node@e62d1c64a114b2d4b223e6e62319cbfa45a42c6a # v4 | |
| with: | |
| node-version: "22" | |
| - name: install typescript | |
| run: npm install --no-save typescript | |
| - name: syntax-check all .ts files | |
| run: node scripts/ts_syntax_check.mjs --json | |
| continue-on-error: true |
🤖 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 @.github/workflows/curriculum.yml around lines 81 - 94, The typescript-syntax
job is labeled advisory but will fail the workflow because the step running node
scripts/ts_syntax_check.mjs exits non-zero on errors; update the
typescript-syntax job to be non-blocking by either (A) replacing the single-step
run of node scripts/ts_syntax_check.mjs in the "syntax-check all .ts files" step
with a small wrapper that captures the JSON output from
scripts/ts_syntax_check.mjs, parses it and prints warnings (mirroring the
python-syntax handling), and exits 0, or (B) simpler: set continue-on-error:
true on the "syntax-check all .ts files" step so failures are advisory; modify
the typescript-syntax job/step accordingly.
Comment on lines
+31
to
+38
| - repo: https://github.com/pre-commit/mirrors-mypy | ||
| rev: v1.15.0 | ||
| hooks: | ||
| - id: mypy | ||
| args: [--config-file=pyproject.toml] | ||
| additional_dependencies: [] | ||
| # mypy is advisory only — do not block commits | ||
| pass_filenames: false |
There was a problem hiding this comment.
Misleading advisory comment for mypy hook.
The comment states "mypy is advisory only — do not block commits", but pre-commit hooks block by default when they fail. There is no verbose: true or other mechanism to make this hook non-blocking. If mypy detects type issues, it will prevent the commit.
Either remove the misleading comment or add verbose: true to the hook configuration to make it truly advisory.
📝 Proposed fix options
Option 1: Remove the misleading comment
- repo: https://github.com/pre-commit/mirrors-mypy
rev: v1.15.0
hooks:
- id: mypy
args: [--config-file=pyproject.toml]
additional_dependencies: []
- # mypy is advisory only — do not block commits
pass_filenames: falseOption 2: Make it truly advisory with verbose mode
- repo: https://github.com/pre-commit/mirrors-mypy
rev: v1.15.0
hooks:
- id: mypy
args: [--config-file=pyproject.toml]
additional_dependencies: []
# mypy is advisory only — do not block commits
+ verbose: true
pass_filenames: false📝 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
| - repo: https://github.com/pre-commit/mirrors-mypy | |
| rev: v1.15.0 | |
| hooks: | |
| - id: mypy | |
| args: [--config-file=pyproject.toml] | |
| additional_dependencies: [] | |
| # mypy is advisory only — do not block commits | |
| pass_filenames: false | |
| - repo: https://github.com/pre-commit/mirrors-mypy | |
| rev: v1.15.0 | |
| hooks: | |
| - id: mypy | |
| args: [--config-file=pyproject.toml] | |
| additional_dependencies: [] | |
| pass_filenames: false |
🤖 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 @.pre-commit-config.yaml around lines 31 - 38, The comment "mypy is advisory
only — do not block commits" is misleading because the pre-commit hook with id:
mypy currently blocks commits on failure; either remove that comment or make the
hook non-blocking by adding the advisory option (e.g., add verbose: true or set
the hook to not fail) to the mypy hook configuration; update the block that
contains repo: https://github.com/pre-commit/mirrors-mypy, rev: v1.15.0, hooks
-> - id: mypy to reflect the chosen change.
Comment on lines
+41
to
+42
| test: | ||
| $(PYTHON) -m pytest --timeout=60 -x |
There was a problem hiding this comment.
Timeout inconsistency between Makefile and pyproject.toml.
The Makefile sets --timeout=60 here, but pyproject.toml line 74 sets timeout = 30 with the comment "Default timeout of 30s — most lesson code is stdlib and fast." This inconsistency could cause confusion about which timeout applies when running tests via different entry points.
Consider aligning both values or documenting why the Makefile uses a longer timeout.
🔧 Proposed fix to align with pyproject.toml
test:
- $(PYTHON) -m pytest --timeout=60 -x
+ $(PYTHON) -m pytest --timeout=30 -xAlternatively, if 60s is intentional for the make test workflow, add a comment explaining the override.
📝 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
| test: | |
| $(PYTHON) -m pytest --timeout=60 -x | |
| test: | |
| $(PYTHON) -m pytest --timeout=30 -x |
🤖 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 `@Makefile` around lines 41 - 42, The Makefile test target uses "--timeout=60"
which conflicts with pyproject.toml's "timeout = 30"; update the Makefile test
target (the "test:" rule invoking $(PYTHON) -m pytest --timeout=60 -x) to use
the same timeout value as pyproject.toml (30) or, if 60s is intentional, add a
brief comment above the "test:" target explaining why this override is required
so maintainers understand the discrepancy.
Comment on lines
+70
to
+72
| scaffold: | ||
| test -n "$(SLUG)" || (echo "Usage: make scaffold PHASE=NN SLUG=lesson-name"; exit 1) | ||
| bash scripts/scaffold-lesson.sh $(PHASE) $(SLUG) |
There was a problem hiding this comment.
Validate PHASE variable in scaffold target.
The usage message on line 71 indicates both PHASE and SLUG are required, but only SLUG is validated. If PHASE is unset, line 72 will pass an empty argument to scaffold-lesson.sh, resulting in a confusing error.
✅ Proposed fix to validate both arguments
scaffold:
+ test -n "$(PHASE)" || (echo "Usage: make scaffold PHASE=NN SLUG=lesson-name"; exit 1)
test -n "$(SLUG)" || (echo "Usage: make scaffold PHASE=NN SLUG=lesson-name"; exit 1)
bash scripts/scaffold-lesson.sh $(PHASE) $(SLUG)📝 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
| scaffold: | |
| test -n "$(SLUG)" || (echo "Usage: make scaffold PHASE=NN SLUG=lesson-name"; exit 1) | |
| bash scripts/scaffold-lesson.sh $(PHASE) $(SLUG) | |
| scaffold: | |
| test -n "$(PHASE)" || (echo "Usage: make scaffold PHASE=NN SLUG=lesson-name"; exit 1) | |
| test -n "$(SLUG)" || (echo "Usage: make scaffold PHASE=NN SLUG=lesson-name"; exit 1) | |
| bash scripts/scaffold-lesson.sh $(PHASE) $(SLUG) |
🤖 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 `@Makefile` around lines 70 - 72, The scaffold Makefile target currently only
validates SLUG; add validation for PHASE as well so both required variables are
checked before calling scaffold-lesson.sh: in the scaffold target (symbol:
scaffold) ensure you test -n "$(PHASE)" and test -n "$(SLUG)" and emit the usage
message "Usage: make scaffold PHASE=NN SLUG=lesson-name" and exit non‑zero if
either is missing, then invoke bash scripts/scaffold-lesson.sh $(PHASE) $(SLUG)
only when both are present.
Comment on lines
+148
to
+152
| if args.missing: | ||
| for c in coverages: | ||
| for name in c.missing: | ||
| print(f"phases/{c.phase_id:02d}-{c.phase_name.lower().replace(' ', '-')}/{name}") | ||
| return 0 |
There was a problem hiding this comment.
Phase slug reconstruction may not match the original directory name.
Line 151 reconstructs the phase directory slug as c.phase_name.lower().replace(' ', '-'), but c.phase_name came from _phase_slug_to_name() which capitalizes each word. This round-trip transformation is lossy and may not produce the original slug.
For example, if the original directory is 01-mlops-foundations, _phase_slug_to_name() produces "Mlops Foundations", and then .lower().replace(' ', '-') produces "mlops-foundations" (missing the original hyphen placement).
🔧 Proposed fix
Store the original phase directory name in PhaseCoverage and use it directly:
`@dataclass`
class PhaseCoverage:
phase_id: int
phase_name: str
+ phase_slug: str
total_lessons: int
with_quiz: int
missing: list[str] phases[phase_id] = PhaseCoverage(
phase_id=phase_id,
phase_name=name,
+ phase_slug=phase_dir.name,
total_lessons=total,
with_quiz=with_quiz,
missing=missing,
) if args.missing:
for c in coverages:
for name in c.missing:
- print(f"phases/{c.phase_id:02d}-{c.phase_name.lower().replace(' ', '-')}/{name}")
+ print(f"phases/{c.phase_slug}/{name}")
return 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 `@scripts/quiz_coverage.py` around lines 148 - 152, The printed phase path
reconstruction is lossy—stop rebuilding the slug from c.phase_name; instead
preserve and use the original directory slug in PhaseCoverage. Update the code
that builds PhaseCoverage (e.g., constructor/factory for PhaseCoverage and the
function that maps slugs to names like _phase_slug_to_name) to store the
original slug into a field (e.g., phase_slug or original_slug), and then change
the printing loop (the block iterating coverages and using c.phase_id and
c.phase_name) to use that stored slug (c.phase_slug) when formatting the output
path so the exact original directory name is emitted.
Comment on lines
+83
to
+87
| lines.append(f"# To import the code under test, add the lesson") | ||
| lines.append(f"# directory to sys.path or install as a package:") | ||
| lines.append(f"# import sys") | ||
| lines.append(f"# sys.path.insert(0, '{rel}/code')") | ||
| lines.append(f"# from {fname.replace('.py', '')} import {', '.join(py_files[fname][:5])}") |
There was a problem hiding this comment.
Remove unnecessary f-string prefixes.
Lines 83-85 use f-string syntax but contain no placeholders. Ruff correctly flags these as unnecessary.
🔧 Proposed fix
lines.append("")
- lines.append(f"# To import the code under test, add the lesson")
- lines.append(f"# directory to sys.path or install as a package:")
- lines.append(f"# import sys")
+ lines.append("# To import the code under test, add the lesson")
+ lines.append("# directory to sys.path or install as a package:")
+ lines.append("# import sys")
lines.append(f"# sys.path.insert(0, '{rel}/code')")
lines.append(f"# from {fname.replace('.py', '')} import {', '.join(py_files[fname][:5])}")
break # Only show the import hint for the first file📝 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
| lines.append(f"# To import the code under test, add the lesson") | |
| lines.append(f"# directory to sys.path or install as a package:") | |
| lines.append(f"# import sys") | |
| lines.append(f"# sys.path.insert(0, '{rel}/code')") | |
| lines.append(f"# from {fname.replace('.py', '')} import {', '.join(py_files[fname][:5])}") | |
| lines.append("") | |
| lines.append("# To import the code under test, add the lesson") | |
| lines.append("# directory to sys.path or install as a package:") | |
| lines.append("# import sys") | |
| lines.append(f"# sys.path.insert(0, '{rel}/code')") | |
| lines.append(f"# from {fname.replace('.py', '')} import {', '.join(py_files[fname][:5])}") |
🧰 Tools
🪛 Ruff (0.15.15)
[error] 83-83: f-string without any placeholders
Remove extraneous f prefix
(F541)
[error] 84-84: f-string without any placeholders
Remove extraneous f prefix
(F541)
[error] 85-85: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 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/scaffold_tests.py` around lines 83 - 87, In the lines that call
lines.append which build the comment block (the three calls that currently use
f-strings but contain no placeholders), remove the unnecessary leading "f" so
they become normal string literals (keep the final lines.append that uses
fname/rel/py_files as an f-string since it contains placeholders); look for the
lines.append(...) calls that reference rel, fname and py_files to identify the
correct statements and change only the f-prefixed literals that have no
interpolation.
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
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.
Summary
This PR fixes systemic quality gaps in the curriculum while preserving all existing content and code style.
Problems addressed
python-syntaxandtypescript-syntaxadvisory jobsquiz-coverageCI job +scaffold_quizzes.pyscriptpytestCI matrix +scaffold_tests.pyscript + 45 example testspyproject.toml(ruff, mypy, pytest).pre-commit-config.yamlMakefilewith 12 targetsFiles changed (11 files, +1,525 / -8)
New (9):
pyproject.toml— ruff, mypy, pytest config.pre-commit-config.yaml— linting & formatting hooksMakefile— 12 common command targetsscripts/quiz_coverage.py— per-phase quiz gap reportingscripts/scaffold_quizzes.py— quiz.json skeleton generatorscripts/scaffold_tests.py— pytest skeleton generator with AST analysisscripts/ts_syntax_check.mjs— TypeScript syntax checker (no dep resolution needed)phases/01.../tests/test_main.py— 26 linear algebra tests (all pass)phases/14.../tests/test_main.py— 19 agent loop tests (all pass)Modified (2):
.github/workflows/curriculum.yml— 4 new CI jobs (python-syntax, ts-syntax, quiz-coverage, pytest)LESSON_TEMPLATE.md— fixed to match actual lesson structureVerification
Non-destructive
🤖 Generated with Claude Code