Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Remi-Gau - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Implementation of
copy_filesis pending; ensure it's completed. (link)
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue, 1 other issue
- 🟢 Security: all looks good
- 🟡 Testing: 4 issues found
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
cohort_creator/copy_files.py
Outdated
| def copy_files( | ||
| output_dir: Path, | ||
| datasets: pd.DataFrame, | ||
| participants: pd.DataFrame | None, | ||
| dataset_types: list[str], | ||
| datatypes: list[str], | ||
| task: str, | ||
| space: str, | ||
| bids_filter: None | dict[str, dict[str, dict[str, str]]] = None, |
There was a problem hiding this comment.
suggestion (code_clarification): Consider documenting the copy_files function.
cohort_creator/copy_files.py
Outdated
| space: str, | ||
| bids_filter: None | dict[str, dict[str, dict[str, str]]] = None, | ||
| ): | ||
| pass |
There was a problem hiding this comment.
issue (code_refinement): Implementation of copy_files is pending; ensure it's completed.
|
|
||
| assert ( | ||
| output_dir / "study-ds000001" / "bids" / "sub-01" / "anat" / "sub-03_T1w.nii.gzz" | ||
| ).exists() |
There was a problem hiding this comment.
suggestion (testing): Test test_copy_files lacks validation for non-existent files.
It would be beneficial to include a test case that validates the behavior when the expected output files do not exist after the copy_files operation. This could help in ensuring the function's robustness in handling errors or unexpected conditions.
| ).exists() | |
| assert not ( | |
| output_dir / "study-ds000001" / "bids" / "sub-01" / "anat" / "sub-03_T1w.nii.gzz" | |
| ).exists() |
tests/test_copy_files.py
Outdated
|
|
||
| import pandas as pd | ||
|
|
||
| from cohort_creator.copy_files import copy_files |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for copy_top_files function.
The test_copy_top_files function currently does not assert any outcomes or behaviors. Adding assertions to verify that the expected top files are copied correctly would enhance the test's effectiveness and coverage.
|
|
||
| assert ( | ||
| output_dir / "study-ds000001" / "bids" / "sub-01" / "anat" / "sub-03_T1w.nii.gzz" | ||
| ).exists() |
There was a problem hiding this comment.
issue (testing): The assertion in test_copy_files may reference an incorrect file path.
The file path in the assertion (sub-03_T1w.nii.gzz) does not match the participant ID (sub-01) used in the test setup. This discrepancy could lead to false positives or negatives in test outcomes. Please verify the intended file path and participant ID.
|
copy_file only works with URL, so this won't work need to implement shrinky: https://github.com/datalad/shrinky |
1 participant
No description provided.