Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (3)
📝 WalkthroughWalkthroughUpdated Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
f98f6db to
5245c8e
Compare
3c51270 to
88ee1de
Compare
5245c8e to
06e1fab
Compare
88ee1de to
df39e5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
divref/tests/conftest.py (1)
24-26: Consider adding a brief docstring for thedatadirfixture.Per coding guidelines, public functions/fixtures benefit from docstrings explaining their purpose.
📝 Proposed docstring
`@pytest.fixture` def datadir() -> Path: + """Return the path to the test data directory.""" return Path(__file__).parent / "data"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@divref/tests/conftest.py` around lines 24 - 26, Add a brief docstring to the datadir pytest fixture that describes its purpose and return value: explain that datadir returns a pathlib.Path pointing to the tests "data" directory adjacent to conftest.py so tests can load test data files; place the docstring immediately below the def datadir(...) signature.divref/tests/tools/test_rewrite_fasta.py (1)
1-53: Consider adding an error-case test for missing input file.Per coding guidelines, new public functions require "at least one happy-path test + one error case." The happy-path coverage is excellent, but there's no test verifying behavior when the input file doesn't exist (expected:
FileNotFoundError).🧪 Suggested error case test
def test_raises_on_missing_input(tmp_path: Path) -> None: with pytest.raises(FileNotFoundError): rewrite_fasta(fasta_path=tmp_path / "nonexistent.fa", output_path=tmp_path / "out.fa")As per coding guidelines: "New public functions require at least one happy-path test + one error case."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@divref/tests/tools/test_rewrite_fasta.py` around lines 1 - 53, Add an error-case test that asserts rewrite_fasta raises FileNotFoundError when the input FASTA is missing: create a new test function (e.g., test_raises_on_missing_input) that calls rewrite_fasta(fasta_path=tmp_path / "nonexistent.fa", output_path=tmp_path / "out.fa") inside a pytest.raises(FileNotFoundError) context using the existing tmp_path fixture; reference the rewrite_fasta function to locate where behavior is expected to raise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@divref/divref/tools/rewrite_fasta.py`:
- Line 24: Change the bare index access on the line sentinel to guard against
empty lines: replace the condition using line[0] (in the loop that processes
FASTA lines) with a check that the line is non-empty before inspecting the first
character (e.g., use line and line[0] == ">" or use line.startswith(">")).
Update the FASTA-reading loop in rewrite_fasta.py where the variable line is
tested so blank lines are skipped safely and no IndexError can occur.
---
Nitpick comments:
In `@divref/tests/conftest.py`:
- Around line 24-26: Add a brief docstring to the datadir pytest fixture that
describes its purpose and return value: explain that datadir returns a
pathlib.Path pointing to the tests "data" directory adjacent to conftest.py so
tests can load test data files; place the docstring immediately below the def
datadir(...) signature.
In `@divref/tests/tools/test_rewrite_fasta.py`:
- Around line 1-53: Add an error-case test that asserts rewrite_fasta raises
FileNotFoundError when the input FASTA is missing: create a new test function
(e.g., test_raises_on_missing_input) that calls
rewrite_fasta(fasta_path=tmp_path / "nonexistent.fa", output_path=tmp_path /
"out.fa") inside a pytest.raises(FileNotFoundError) context using the existing
tmp_path fixture; reference the rewrite_fasta function to locate where behavior
is expected to raise.
🪄 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: d5886734-3cee-4947-aa92-b18a1059d690
📒 Files selected for processing (8)
.gitignoredivref/divref/haplotype.pydivref/divref/tools/rewrite_fasta.pydivref/tests/conftest.pydivref/tests/data/test.fadivref/tests/data/test.fa.faidivref/tests/test_haplotype.pydivref/tests/tools/test_rewrite_fasta.py
| keep = False | ||
| with open(fasta_path) as f, open(output_path, "w") as out: | ||
| for line in tqdm.tqdm(f): | ||
| if line[0] == ">": |
There was a problem hiding this comment.
Potential IndexError on empty lines in the FASTA file.
If the input FASTA contains blank lines (e.g., between contigs or trailing newlines), line[0] will raise an IndexError. While well-formed FASTA files typically don't have empty lines, defensive handling would improve robustness.
🛡️ Proposed fix
for line in tqdm.tqdm(f):
+ if not line.strip():
+ continue
if line[0] == ">":🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@divref/divref/tools/rewrite_fasta.py` at line 24, Change the bare index
access on the line sentinel to guard against empty lines: replace the condition
using line[0] (in the loop that processes FASTA lines) with a check that the
line is non-empty before inspecting the first character (e.g., use line and
line[0] == ">" or use line.startswith(">")). Update the FASTA-reading loop in
rewrite_fasta.py where the variable line is tested so blank lines are skipped
safely and no IndexError can occur.
24a7f04 to
a2f8fa4
Compare
Port rewrite_fasta.py from human-diversity-reference/scripts as a defopt-compatible toolkit tool. Filters a FASTA file to keep only canonical chromosomes (chr1-22, X, Y, MT). Fixes a potential UnboundLocalError by initialising keep=False before the loop and removes the unused header_line variable from the original. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds tests/tools/test_rewrite_fasta.py covering rewrite_fasta() using pytest's tmp_path fixture: - Canonical autosomes (chr1, chr22) are kept - Sex chromosomes (chrX, chrY) and chrMT are kept - Alt contigs (chr1_alt) and decoys (chrUn_gl000220, chrEBV) are filtered - Mixed input with canonical and non-canonical contigs interleaved - Empty input produces empty output - Multi-line sequences are written in full Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
a2f8fa4 to
d07c2c6
Compare
Summary
rewrite_fasta.pyfromhuman-diversity-reference/scriptsas a defopt-compatible toolkit toolchr1–22,X,Y,MT)UnboundLocalErrorin the original by initialisingkeep=Falsebefore the loopheader_linevariable from the originalTest plan
uv run --directory divref poe check-allpasses🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests