Removing auto-running of pre-commit, move tests to Makefile #37
Conversation
WalkthroughPre-commit hooks simplified (removed ruff-check, isort, black; kept ruff-format and nbstripout). Added a Makefile with formatting targets and notebook discovery variables. Development requirements now include Changes
Sequence Diagram(s)sequenceDiagram
participant PR as Pull Request
participant GH as GitHub Actions
participant Make as Makefile
participant Tool as Formatting Tools (ruff, nbstripout)
participant Job as Tests / Notebook Execution
PR->>GH: trigger workflow (validate-python / notebooks)
GH->>GH: install formatting deps (ruff, nbstripout)
GH->>Make: run formatting check (make format-python-check / format-notebooks-check)
Make->>Tool: invoke ruff / nbstripout (check/verify)
Tool-->>Make: pass / fail
Make-->>GH: exit status
alt formatting passed
GH->>Job: continue to run tests / execute notebooks
else formatting failed
GH-->>PR: fail workflow (stop)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used🪛 checkmake (0.2.2)Makefile[warning] 1-1: Missing required phony target "all" (minphony) [warning] 1-1: Missing required phony target "clean" (minphony) [warning] 1-1: Missing required phony target "test" (minphony) 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: 1
🧹 Nitpick comments (3)
requirements-dev.txt (1)
1-2: Pin versions for ruff and nbstripout to ensure reproducible installs.The dependencies lack version constraints, which risks installing incompatible versions in the future. For comparison,
.pre-commit-config.yamlexplicitly pins ruff tov0.13.0and nbstripout to0.8.1. Consider adding these (or similar tested versions) torequirements-dev.txtfor reproducibility and consistency.-ruff -nbstripout +ruff==0.13.0 +nbstripout==0.8.1.github/workflows/formatting-tests.yml (1)
39-40: Rename step to reflect actual behavior.The step name "Install pre-commit" is misleading since the step installs
ruffandnbstripout, not pre-commit. Rename to "Install formatting tools" or similar for clarity.- - name: Install pre-commit + - name: Install formatting tools run: pip install ruff nbstripoutMakefile (1)
1-16: Consider adding conventional Makefile targets.Static analysis suggests missing
allandcleanphony targets, which are conventionally present in Makefiles. While not strictly required for this use case, adding them would improve consistency with Makefile conventions:
all: typically the default target (could depend ontest)clean: typically removes generated/temporary files (e.g.,__pycache__,.pyc, etc.)If these targets are not applicable to the project, you may suppress the checkmake warnings; otherwise, consider adding them for completeness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/formatting-tests.yml(3 hunks).pre-commit-config.yaml(1 hunks)Makefile(1 hunks)requirements-dev.txt(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
🔇 Additional comments (2)
.pre-commit-config.yaml (1)
1-23: Clarify intent: are ruff-format and nbstripout hooks meant to persist?The file now retains
ruff-formatandnbstripouthooks while removingruff-check,isort, andblack. Given the PR goal to move testing to Makefile, confirm whether these two hooks should remain as automatic pre-commit checks or also be removed for consistency. If the intent is to let developers opt-in (viamake lint/nbcleanup) rather than auto-run, removing these hooks would align better with the PR objectives.Makefile (1)
3-5: No action needed—notebook paths are complete.The verification confirms all notebooks in the repository are in the two directories specified by the Makefile (
notebooks/use-cases/andnotebooks/tutorials/). The hardcoded paths are sufficient and no additional pattern changes are required.
| - name: Run Formatting Tests | ||
| run: make test |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: Workflow does not verify formatting compliance.
The workflow runs ruff format and nbstripout, which modify files in-place, but doesn't verify the code was properly formatted before the run. This means the workflow will succeed even if formatting violations exist—it silently fixes them without failing the build.
Typical CI workflows should either:
- Use check-mode flags to detect (not fix) formatting issues and fail if found.
- Run formatters, then check for uncommitted changes and fail if any exist.
- Reject PRs with formatting violations.
Please clarify the intended behavior and verify whether ruff format supports a --check flag or similar to detect violations without modifying files. If the goal is to auto-fix, the workflow should commit/push changes; otherwise, it should verify formatting without modifying.
🌐 Web query:
Does ruff format support a --check flag to verify formatting without modifying files?
💡 Result:
Yes. Use ruff format --check (e.g. ruff format --check path/to/file.py). It will not write changes and exits non‑zero if any files would be reformatted. [1][2]
Sources:
[1] Ruff formatter docs — The Ruff Formatter.
[2] Ruff format options (config/help) — Configuring Ruff / ruff format.
🏁 Script executed:
#!/bin/bash
# Find and display the Makefile
fd -name "Makefile" -type f | head -5 | xargs -I {} sh -c 'echo "File: {}"; cat {}'Length of output: 230
🏁 Script executed:
#!/bin/bash
# Also check for makefile (lowercase)
fd -iname "makefile" -type f | head -5Length of output: 230
🏁 Script executed:
#!/bin/bash
# Look for Makefile in root and common locations
echo "=== Searching for Makefile ==="
find . -maxdepth 2 -name "[Mm]akefile" -type f 2>/dev/null
echo "=== Checking workflow file ==="
cat .github/workflows/formatting-tests.yml | sed -n '35,50p'Length of output: 291
🏁 Script executed:
#!/bin/bash
# If Makefile found, display it
if [ -f ./Makefile ]; then
echo "=== Makefile contents ==="
cat ./Makefile
elif [ -f ./makefile ]; then
echo "=== makefile contents ==="
cat ./makefile
else
echo "Makefile not found in root"
fiLength of output: 393
Critical: Workflow auto-fixes formatting instead of enforcing compliance.
The make test target runs ruff format and nbstripout without check flags, which modifies files in-place. CI workflows should verify formatting without modifying files.
Update the Makefile:
- Change
ruff formattoruff format --checkin thelinttarget (exits non-zero if reformatting needed) - Change
nbstripout $(ALL_NOTEBOOKS)tonbstripout --verify $(ALL_NOTEBOOKS)in thenbcleanuptarget (exits 1 if any notebooks would change)
This ensures the workflow fails PRs with formatting violations instead of silently fixing them.
🤖 Prompt for AI Agents
In .github/workflows/formatting-tests.yml around lines 42 to 43, the CI
currently runs `make test` which invokes Makefile targets that auto-fix files;
update the Makefile so the `lint` target uses `ruff format --check` instead of
`ruff format` and the `nbcleanup` target uses `nbstripout --verify
$(ALL_NOTEBOOKS)` instead of `nbstripout $(ALL_NOTEBOOKS)` so the CI verifies
formatting and fails on violations instead of modifying files in-place.
alimaredia
force-pushed
the
formatting-workflows
branch
from
65063d5 to
b3b99a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/formatting-tests.yml(2 hunks).pre-commit-config.yaml(1 hunks)Makefile(1 hunks)requirements-dev.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .pre-commit-config.yaml
- requirements-dev.txt
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
🔇 Additional comments (1)
.github/workflows/formatting-tests.yml (1)
40-44: Workflow depends on Makefile check flags—verify the Makefile targets use--checkand--verify.The workflow structure looks correct, but CI will silently fix formatting violations instead of failing if the Makefile targets don't use check flags. Per the past review, ensure the
linttarget usesruff format --checkand thenbcleanuptarget usesnbstripout --verifyso the workflow verifies compliance and exits non-zero on violations.
Makefile
Outdated
| lint: | ||
| @echo "Running ruff format..." | ||
| ruff format |
There was a problem hiding this comment.
Critical: lint target must verify formatting without modifying files.
Line 9 runs ruff format which modifies files in-place. CI should verify formatting compliance and fail on violations, not auto-fix. Use ruff format --check instead, which exits non-zero if reformatting is needed without modifying files.
Apply this diff:
lint:
@echo "Running ruff format..."
- ruff format
+ ruff format --check📝 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.
| lint: | |
| @echo "Running ruff format..." | |
| ruff format | |
| lint: | |
| @echo "Running ruff format..." | |
| ruff format --check |
🤖 Prompt for AI Agents
In Makefile around lines 7 to 9, the lint target currently runs "ruff format"
which mutates files; change it to run "ruff format --check" so CI verifies
formatting and fails on violations without modifying files, ensuring the lint
target exits non-zero when reformatting is required.
4ff238f to
4f885cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/validate-and-execute-notebooks.yml(2 hunks).github/workflows/validate-python.yml(2 hunks).pre-commit-config.yaml(1 hunks)Makefile(1 hunks)requirements-dev.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .pre-commit-config.yaml
- requirements-dev.txt
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🔇 Additional comments (2)
.github/workflows/validate-python.yml (1)
9-9: Inconsistent path trigger:.pre-commit-config.yamlstill referenced after pre-commit removal.Lines 9 and 16 include
.pre-commit-config.yamlin the workflow trigger paths, but the PR is removing pre-commit entirely. This is inconsistent with the stated PR objective and may cause workflow behavior issues:
- If
.pre-commit-config.yamlis deleted, the workflow will never trigger on Python file changes- If it's kept, this path should be removed since the workflow no longer uses it
Clarify the intent: is
.pre-commit-config.yamlbeing retained or deleted?Also applies to: 16-16
Makefile (1)
3-5: No issues found—glob patterns capture all notebooks.The verification confirms that the glob patterns on lines 3-4 successfully capture all
.ipynbfiles in the repository. All 4 notebooks are matched: 3 innotebooks/use-cases/and 1 innotebooks/tutorials/. There are no additional notebooks in other locations that should be formatted.
| @@ -0,0 +1,21 @@ | |||
| .PHONY: format-python format-notebook format-python-check format-notebooks-check | |||
There was a problem hiding this comment.
Critical: Missing test target blocks PR objectives.
The PR objectives state developers should be able to run make test to run both formatting checks, but this Makefile defines no test target. The static analysis also flags a missing "test" phony target.
Define a test target that runs both Python and notebook formatting checks:
-.PHONY: format-python format-notebook format-python-check format-notebooks-check
+.PHONY: format-python format-notebook format-python-check format-notebooks-check test
USE_CASES := $(wildcard notebooks/use-cases/*.ipynb)
TUTORIALS := $(wildcard notebooks/tutorials/*.ipynb)
ALL_NOTEBOOKS := $(USE_CASES) $(TUTORIALS)
format-python:
@echo "Running ruff format..."
ruff format
format-notebooks:
@echo "Running nbstripout..."
nbstripout $(ALL_NOTEBOOKS)
format-notebooks-check: lint nbcleanup
nbstripout --verify $(ALL_NOTEBOOKS)
@echo "nbstripout check passed :)"
format-python-check: lint nbcleanup
ruff format --check
@echo "ruff format check passed :)"
+
+test: format-python-check format-notebooks-check
+ @echo "All formatting checks passed :)"Also applies to: 7-21
🧰 Tools
🪛 checkmake (0.2.2)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🤖 Prompt for AI Agents
In Makefile around lines 1 to 1, the Makefile lacks a test target required by PR
objectives and static analysis; add a phony "test" target to the .PHONY list and
define a test target that runs both format-python-check and
format-notebooks-check (e.g., make test should invoke those two check targets),
ensuring the new "test" target is listed in .PHONY so make treats it as a phony
target.
🧩 Analysis chain
Critical: Makefile targets depend on undefined lint and nbcleanup targets.
Lines 15 and 19 declare dependencies on lint and nbcleanup targets, but these targets are not defined anywhere in the provided Makefile. This will cause both workflows to fail with:
make: *** No rule to make target 'lint'. Stop.
make: *** No rule to make target 'nbcleanup'. Stop.
The .PHONY declaration (line 1) also doesn't include these dependencies, suggesting they may be missing entirely.
Also applies to: 15-15, 19-19
🏁 Script executed:
#!/bin/bash
# Check if lint and nbcleanup targets are defined in Makefile
echo "=== Checking for lint target ==="
grep -n "^lint:" Makefile || echo "NOT FOUND"
echo "=== Checking for nbcleanup target ==="
grep -n "^nbcleanup:" Makefile || echo "NOT FOUND"
echo "=== Full Makefile for context ==="
cat MakefileLength of output: 704
Makefile has undefined target dependencies that will cause failures on lines 15 and 19.
Both format-notebooks-check and format-python-check targets depend on lint and nbcleanup, but neither of these targets is defined in the Makefile. This will cause Make to fail immediately with "No rule to make target 'lint'" when either check target is invoked.
Additionally, the .PHONY declaration on line 1 is incomplete:
- Includes
format-notebook(singular) but the actual target isformat-notebooks(plural) - Missing
format-notebooks,lint, andnbcleanupdeclarations
Define the missing lint and nbcleanup targets, fix the .PHONY declaration, and ensure both check targets have valid dependencies.
🧰 Tools
🪛 checkmake (0.2.2)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
There was a problem hiding this comment.
Like coderabbit pointed out, looks like these are more of dependencies and should be cleared
There was a problem hiding this comment.
@shruthis4, you're absolutely right! Those undefined dependencies need to be removed and the targets properly structured. Here's a comprehensive fix that addresses all the issues:
.PHONY: format-python format-notebooks format-python-check format-notebooks-check lint nbclean test
USE_CASES := $(wildcard notebooks/use-cases/*.ipynb)
TUTORIALS := $(wildcard notebooks/tutorials/*.ipynb)
ALL_NOTEBOOKS := $(USE_CASES) $(TUTORIALS)
format-python:
@echo "Running ruff format..."
ruff format
format-notebooks:
@echo "Running nbstripout..."
nbstripout $(ALL_NOTEBOOKS)
format-notebooks-check:
@echo "Running nbstripout verification..."
nbstripout --verify $(ALL_NOTEBOOKS)
@echo "✓ nbstripout check passed"
format-python-check:
@echo "Running ruff format check..."
ruff format --check
@echo "✓ ruff format check passed"
# User-facing targets per PR objectives
lint: format-python-check
@echo "✓ Lint check complete"
nbclean: format-notebooks-check
@echo "✓ Notebook cleanup check complete"
test: lint nbclean
@echo "✓ All formatting checks passed"Key changes:
- Removed undefined dependencies from lines 15 and 19 (
lintandnbcleanup) - Fixed
.PHONYdeclaration - changedformat-notebooktoformat-notebooks(plural) and addedlint,nbclean, andtest - Added the three targets from PR objectives:
make lint→ runs Python formatting checkmake nbclean→ runs notebook cleanup checkmake test→ runs both checks
- Cleaner echo messages with checkmarks for better UX
This structure aligns with the PR objectives and eliminates the circular/undefined dependency issues! 🎯
alimaredia
force-pushed
the
formatting-workflows
branch
2 times, most recently
from
a22fb19 to
874c849
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Makefile (1)
1-1: Critical: Makefile does not implement PR objectives; missinglint,nbclean, andtesttargets.The PR explicitly states developers should run:
make lintto run ruffmake nbcleanto run nbstripoutmake testto run bothHowever, the Makefile defines
format-python,format-notebooks,format-python-check, andformat-notebooks-checkinstead. None of the required targets exist.Additionally, line 1 declares
.PHONY: format-notebook(singular) but the actual target isformat-notebooks(plural at line 11).Define the missing targets and fix the
.PHONYdeclaration:-.PHONY: format-python format-notebook format-python-check format-notebooks-check +.PHONY: format-python format-notebooks format-python-check format-notebooks-check lint nbclean test USE_CASES := $(wildcard notebooks/use-cases/*.ipynb) TUTORIALS := $(wildcard notebooks/tutorials/*.ipynb) ALL_NOTEBOOKS := $(USE_CASES) $(TUTORIALS) format-python: @echo "Running ruff format..." ruff format -format-notebooks: +format-notebooks: @echo "Running nbstripout..." nbstripout $(ALL_NOTEBOOKS) format-notebooks-check: nbstripout --verify $(ALL_NOTEBOOKS) @echo "nbstripout check passed :)" format-python-check: ruff format --check @echo "ruff format check passed :)" + +lint: format-python-check + @echo "Lint passed :)" + +nbclean: format-notebooks + @echo "Notebooks cleaned :)" + +test: format-python-check format-notebooks-check + @echo "All formatting checks passed :)"
🧹 Nitpick comments (1)
requirements-dev.txt (1)
1-2: Consider pinning versions for reproducibility.Adding version constraints (e.g.,
ruff>=0.7.0,nbstripout>=0.6.0) ensures consistent behavior across developer machines and CI runs. Without constraints, a major version bump could introduce breaking changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/validate-and-execute-notebooks.yml(2 hunks).github/workflows/validate-python.yml(2 hunks).pre-commit-config.yaml(1 hunks)Makefile(1 hunks)requirements-dev.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/validate-python.yml
- .github/workflows/validate-and-execute-notebooks.yml
- .pre-commit-config.yaml
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 GitHub Actions: Validation & Smoke Tests for Notebooks
Makefile
[error] 1-1: Command failed: make format-notebooks-check. Exit code 2. Dry-run notes indicate nbstripout verification failed for notebooks.
🔇 Additional comments (1)
Makefile (1)
11-17: The Makefile and workflow are correctly configured; however, I cannot independently verify the notebook state.The
make format-notebooks-checktarget is properly set up in the Makefile (lines 16-17) and is actually executed by the CI workflow (validate-and-execute-notebooks.yml, in the execute_tests job). The workflow installs nbstripout and runs the verification.Since I cannot run nbstripout in the sandbox environment, I cannot confirm whether notebooks are currently failing the check. The original concern is valid and plausible—if
nbstripout --verifyis failing in CI, notebooks need to be cleaned locally.To resolve this:
- Run
make format-notebookslocally to strip all notebooks- Verify with
make format-notebooks-check- Commit the cleaned notebooks
The provided verification script is redundant; the CI workflow already performs this check.
alimaredia
force-pushed
the
formatting-workflows
branch
from
874c849 to
96e6380
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/validate-and-execute-notebooks.yml(2 hunks).github/workflows/validate-python.yml(2 hunks).pre-commit-config.yaml(1 hunks)Makefile(1 hunks)requirements-dev.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/validate-python.yml
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🔇 Additional comments (3)
requirements-dev.txt (1)
1-2: Dependencies align with Makefile formatting targets.The additions support the formatting targets introduced in the Makefile and referenced in updated workflows.
.pre-commit-config.yaml (1)
1-23: Pre-commit configuration correctly aligned with Makefile migration.Removal of ruff-check, isort, and black hooks is consistent with the PR objective to move formatting validation from pre-commit to Makefile/CI. Retained ruff-format and nbstripout hooks are appropriate for local formatting enforcement.
.github/workflows/validate-and-execute-notebooks.yml (1)
106-112: Workflow integration with Makefile is properly structured.The formatting validation step correctly invokes
make format-notebooks-check, which is defined in the Makefile. Once Makefile issues are resolved, this step will execute as intended. The addition ofnbstripoutto dependencies (line 108) appropriately supports this step.Please verify that the Makefile targets match the intended developer-facing API as stated in the PR objectives.
| .PHONY: format-python format-notebook format-python-check format-notebooks-check | ||
|
|
||
| USE_CASES := $(wildcard notebooks/use-cases/*.ipynb) | ||
| TUTORIALS := $(wildcard notebooks/tutorials/*.ipynb) | ||
| ALL_NOTEBOOKS := $(USE_CASES) $(TUTORIALS) | ||
|
|
||
| COMMON := $(wildcard kubeflow-pipelines/common/*.py) | ||
| DOCLING_STANDARD_PIPELINE := $(wildcard kubeflow-pipelines/docling-standard/*.py) | ||
| DOCLING_VLM_PIPELINE := $(wildcard kubeflow-pipelines/docling-vlm/*.py) | ||
| ALL_PYTHON_FILES := $(COMMON) $(DOCLING_STANDARD_PIPELINE) $(DOCLING_VLM_PIPELINE) | ||
|
|
||
| format-python: | ||
| @echo "Running ruff format..." | ||
| ruff format $(ALL_PYTHON_FILES) | ||
|
|
||
| format-notebooks: | ||
| @echo "Running nbstripout..." | ||
| nbstripout $(ALL_NOTEBOOKS) | ||
|
|
||
| format-notebooks-check: | ||
| nbstripout --verify $(ALL_NOTEBOOKS) | ||
| @echo "nbstripout check passed :)" | ||
|
|
||
| format-python-check: | ||
| ruff format --check $(ALL_PYTHON_FILES) | ||
| @echo "ruff format check passed :)" |
There was a problem hiding this comment.
Critical: Makefile targets do not match PR objectives.
The PR description states developers should be able to run make lint, make nbclean, and make test, but the Makefile defines format-python, format-notebooks, format-python-check, and format-notebooks-check instead. This represents a significant discrepancy between stated requirements and implementation.
Additionally:
- Missing
testtarget (line 1, static analysis): The.PHONYdeclaration and Makefile lack atesttarget required by PR objectives and static analysis checks. .PHONYdeclaration mismatch (line 1): Listsformat-notebook(singular) but the actual target isformat-notebooks(plural, line 16). Missing declarations forformat-notebooksandtest.
Either:
- Rename Makefile targets to match PR objectives (
lint,nbclean,test), or - Update PR description and documentation to reflect the actual target names (
format-python,format-notebooks,format-python-check,format-notebooks-check)
Additionally, define a test target that runs both check targets as specified in PR objectives.
Proposed fix (if aligning to PR objectives):
-.PHONY: format-python format-notebook format-python-check format-notebooks-check
+.PHONY: format-python format-notebooks format-python-check format-notebooks-check lint nbclean test
USE_CASES := $(wildcard notebooks/use-cases/*.ipynb)
TUTORIALS := $(wildcard notebooks/tutorials/*.ipynb)
ALL_NOTEBOOKS := $(USE_CASES) $(TUTORIALS)
@@ -12,6 +12,14 @@ format-notebooks:
@echo "Running nbstripout..."
nbstripout $(ALL_NOTEBOOKS)
+lint: format-python
+ @echo "lint target complete"
+
+nbclean: format-notebooks
+ @echo "nbclean target complete"
+
+test: format-python-check format-notebooks-check
+ @echo "All formatting checks passed :)"
+
format-notebooks-check:
nbstripout --verify $(ALL_NOTEBOOKS)
@echo "nbstripout check passed :)"Alternatively, if the naming is intentional, update the PR description to reflect the actual interface and verify all documentation/workflows reference the correct target names.
🧰 Tools
🪛 checkmake (0.2.2)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🤖 Prompt for AI Agents
In Makefile around lines 1-26, the declared targets and .PHONY list don't match
the PR requirements: add pragmatic targets named lint, nbclean, and test, fix
the .PHONY entry to list the actual target names (use plural form
format-notebooks if kept), and make test run the existing check targets;
specifically, keep the current format-* targets for backward compatibility,
update .PHONY to ".PHONY: format-python format-notebooks format-python-check
format-notebooks-check lint nbclean test", add a lint target that runs ruff
format --check on ALL_PYTHON_FILES (and echoes a success message), add an
nbclean target that runs nbstripout on ALL_NOTEBOOKS (and echoes a success
message), and add a test target that invokes the two check targets
(format-python-check and format-notebooks-check) so make test executes both
checks.
This commit changes the harness running ruff
and nbstripout from pre-commit to a Makefile.
Developers can run `make format-python` to run `ruff format`
and format python files.
Developers can also run `make format-notebooks` to
run `nbstripout` and format notebooks.
`make format-{notebook,python}-check`
checks that files have been appropriately formatted.
These checks are run as workflows when python or
notebooks files are changed.
Signed-off-by: Ali Maredia <[email protected]>
alimaredia
force-pushed
the
formatting-workflows
branch
from
96e6380 to
dca2d44
Compare
| @@ -0,0 +1,21 @@ | |||
| .PHONY: format-python format-notebook format-python-check format-notebooks-check | |||
There was a problem hiding this comment.
Like coderabbit pointed out, looks like these are more of dependencies and should be cleared
2 participants
This PR changes the harness running ruff and nbstripout from pre-commit to a Makefile.
Developers can run
make format-pythonto runruff formatand format python files.Developers can also run
make format-notebooksto runnbstripoutand format notebooks.make format-{notebook,python}-checkchecks that files have been appropriately formatted.These checks are run as workflows when python or notebooks files are changed.
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit