Coverage #56
Coverage #56
Conversation
Reviewer's Guide by SourceryThis pull request focuses on improving code coverage reporting. It configures the coverage check workflow to run on multiple Python versions, modifies the Updated class diagram for ExampleDirclassDiagram
class ExampleDir{
+path: Path
+pytester: Pytester
+example_dir_cache: dict
+conftest: str
+ini: str
+files: list[str]
+__init__(path: Path, pytester: Pytester, example_dir_cache: dict, conftest: str, ini: str, files: list[str])
}
note for ExampleDir "Added pragma: no cover"
class ExampleDirRequest{
+param: ExampleDirSpec
+__init__(param: ExampleDirSpec)
}
note for ExampleDirRequest "No changes"
class ExampleDirSpec{
+path: str
+conftest: str
+ini: str
+files: list[str]
+__init__(path: str, conftest: str, ini: str, files: list[str])
}
note for ExampleDirSpec "No changes"
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #56 +/- ##
============================================
+ Coverage 71.91% 100.00% +28.08%
============================================
Files 4 4
Lines 349 340 -9
Branches 29 30 +1
============================================
+ Hits 251 340 +89
+ Misses 97 0 -97
+ Partials 1 0 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @MusicalNinjaDad - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using
fail-fast: truein the matrix strategy to avoid running all Python versions if one fails. - The
dynamic_contextconfiguration inpyproject.tomlmight not be necessary given the context is already set in the command line.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Review instructions: 5 issues found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| uv run coverage erase | ||
| rm -rf pycov | ||
| uv run --python 3.13 coverage run --context=3.13 -m pytest | ||
| UV_PROJECT_ENVIRONMENT="./.venv-3.12" uv run --python 3.12 coverage run --context=3.12 --append -m pytest | ||
| UV_PROJECT_ENVIRONMENT="./.venv-3.11" uv run --python 3.11 coverage run --context=3.11 --append -m pytest | ||
| UV_PROJECT_ENVIRONMENT="./.venv-3.10" uv run --python 3.10 coverage run --context=3.10 --append -m pytest | ||
| UV_PROJECT_ENVIRONMENT="./.venv-3.9" uv run --python 3.9 coverage run --context=3.9 --append -m pytest | ||
| uv run coverage report | ||
| uv run coverage html --show-contexts -d pycov |
There was a problem hiding this comment.
suggestion: Multiple similar coverage commands in the 'cov' target.
The repetitive commands for different Python versions work correctly, but consider refactoring or abstracting the common parts to simplify future maintenance if additional versions are supported.
Suggested implementation:
cov:
uv run coverage erase
rm -rf pycov
for ver in 3.13 3.12 3.11 3.10 3.9; do \
if [ "$ver" = "3.13" ]; then \
uv run --python $ver coverage run --context=$ver -m pytest; \
else \
UV_PROJECT_ENVIRONMENT="./.venv-$ver" uv run --python $ver coverage run --context=$ver --append -m pytest; \
fi; \
done
uv run coverage report
uv run coverage html --show-contexts -d pycov
Ensure that your environment supports executing shell loops within the justfile. If not, you may need to wrap this loop in an external shell script and call it from the justfile.
| @@ -21,7 +21,7 @@ | |||
| with suppress(ImportError): | |||
| from typing import Self # not type-checking on python < 3.11 so don't care if this fails | |||
|
|
|||
| if sys.version_info < (3, 10): # dataclass does not offer kw_only on python < 3.10 # pragma: no cover | |||
| if sys.version_info < (3, 10): # dataclass does not offer kw_only on python < 3.10 | |||
There was a problem hiding this comment.
suggestion (review_instructions): Consider using EAFP instead of LBYL for version checking
This version check is using Look Before You Leap (LBYL) pattern. Consider using Easier to Ask for Forgiveness than Permission (EAFP) by trying to import/use the feature directly and catching the exception if it fails. For example: try: ... except TypeError: # kw_only not supported
Review instructions:
Path patterns: **/*.py, **/*.ipynb
Instructions:
Prefer EAFP to LBYL in most cases
| @@ -21,7 +21,7 @@ | |||
|
|
|||
| CELL_PREFIX: Final[str] = "Cell" | |||
|
|
|||
| if sys.version_info < (3, 12): # pragma: no cover | |||
| if sys.version_info < (3, 12): | |||
There was a problem hiding this comment.
suggestion (review_instructions): Consider using EAFP instead of LBYL for version checking
This version check is using Look Before You Leap (LBYL) pattern. Consider using Easier to Ask for Forgiveness than Permission (EAFP) by trying to use the feature directly and catching the exception if it fails.
Review instructions:
Path patterns: **/*.py, **/*.ipynb
Instructions:
Prefer EAFP to LBYL in most cases
| @@ -43,7 +43,7 @@ def exists(self, *args: Any, **kwargs: Any) -> bool: | |||
| # TODO: #33 Extend `CellPath.exists` to also check that the cell exists (if performance allows) | |||
| return self.notebook.exists(*args, **kwargs) | |||
|
|
|||
| if sys.version_info < (3, 13): # pragma: no cover | |||
| if sys.version_info < (3, 13): | |||
There was a problem hiding this comment.
suggestion (review_instructions): Consider using EAFP instead of LBYL for version checking
This version check is using Look Before You Leap (LBYL) pattern. Consider using Easier to Ask for Forgiveness than Permission (EAFP) by trying to use the feature directly and catching the exception if it fails.
Review instructions:
Path patterns: **/*.py, **/*.ipynb
Instructions:
Prefer EAFP to LBYL in most cases
| @@ -257,10 +257,10 @@ def example_dir( | |||
| example = request.param | |||
| if (cached_dir := example_dir_cache.get(example)) is None: | |||
| (pytester.path / example.path).mkdir(parents=True, exist_ok=True) | |||
| if example.conftest: | |||
| if example.conftest: # pragma: no cover | |||
There was a problem hiding this comment.
suggestion (review_instructions): Consider using EAFP instead of LBYL for attribute checking
This is using Look Before You Leap (LBYL) to check if the attribute exists or has a truthy value. Consider if an EAFP approach would be more appropriate here.
Review instructions:
Path patterns: **/*.py, **/*.ipynb
Instructions:
Prefer EAFP to LBYL in most cases
| pytester.makeconftest(request.param.conftest) | ||
|
|
||
| if example.ini: | ||
| if example.ini: # pragma: no cover |
There was a problem hiding this comment.
suggestion (review_instructions): Consider using EAFP instead of LBYL for attribute checking
This is using Look Before You Leap (LBYL) to check if the attribute exists or has a truthy value. Consider if an EAFP approach would be more appropriate here.
Review instructions:
Path patterns: **/*.py, **/*.ipynb
Instructions:
Prefer EAFP to LBYL in most cases
Fixes #53
Helps towards #34
Summary by Sourcery
Improve test coverage reporting by configuring the CI workflow to run coverage on multiple Python versions and upload the results to Codecov. Also, update the justfile to run coverage for all python versions.
CI:
Tests: