feat: add index option to case import #136
Conversation
|
Warning Rate limit exceeded@tedil has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce a new command-line option to the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
varfish_cli/cli/importer/__init__.py(2 hunks)varfish_cli/cli/importer/create.py(3 hunks)
🔇 Additional comments (4)
varfish_cli/cli/importer/__init__.py (3)
Line range hint 136-155: Implementation successfully addresses PR objectives.
The changes effectively implement the requested feature to specify an index patient during import while maintaining backward compatibility. The implementation is clean and minimally invasive.
155-155: Verify index parameter handling in CaseImportOptions.
The parameter is correctly passed to CaseImportOptions. However, please ensure that:
- The index value is properly validated against the pedigree data
- Error handling is in place for invalid index values
Let's verify the implementation in CaseImportOptions:
#!/bin/bash
# Search for index handling in CaseImportOptions
ast-grep --pattern 'class CaseImportOptions {
$$$
def __init__($$$) {
$$$
self.index = $$$
$$$
}
$$$
}'136-142: Enhance parameter documentation and validation.
The index parameter implementation looks good, but consider these improvements:
- Expand the help text to clarify the exact format required (e.g., "Must match a patient name in the pedigree file").
- Add runtime validation to ensure the provided index exists in the pedigree.
Let's verify if validation is implemented in the CaseImporter class:
✅ Verification successful
Index validation is already implemented, help text could be improved
The code review shows that:
- The index validation is implicitly handled in
_create_case_import_info()where the index is used with the pedigree data. - The pedigree members are validated through
_check_genotypes()which ensures all samples are properly accounted for. - The fallback to first pedigree member (
self.pedigree[0].name) when index is None is correctly implemented.
Suggested improvement:
- Update the help text to: "Name of the index case in the pedigree. Must match a patient name in the pedigree file. Defaults to the first case."
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for index validation in CaseImporter
ast-grep --pattern 'class CaseImporter {
$$$
def _create_case_import_info($$$) {
$$$
if $index $$$
$$$
}
$$$
}'
Length of output: 296
Script:
#!/bin/bash
# Search for index validation in the codebase
rg -A 10 "def _create_case_import_info" varfish_cli/cli/importer/
# Search for CaseImportOptions class
rg -A 10 "class CaseImportOptions" varfish_cli/cli/importer/
# Search for any index validation
rg -B 2 -A 5 "index.*pedigree" varfish_cli/cli/importer/
Length of output: 3501
Script:
#!/bin/bash
# Search for pedigree validation and member name checks
rg -B 2 -A 5 "_load_pedigree" varfish_cli/cli/importer/
# Look for any validation of index against pedigree members
rg -B 2 -A 10 "def _check" varfish_cli/cli/importer/
Length of output: 4397
varfish_cli/cli/importer/create.py (1)
282-282: LGTM: Type annotation for index field is correct.
The optional index field is properly typed as Union[str, None] and follows Python typing conventions.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
If no manual index is specified, ideally the index should be assigned to the first affected sample in the pedigree, not just the first sample overall. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #136 +/- ##
==========================================
- Coverage 83.99% 83.33% -0.67%
==========================================
Files 21 21
Lines 1506 1452 -54
==========================================
- Hits 1265 1210 -55
- Misses 241 242 +1
|
Resolves #133
Summary by CodeRabbit
New Features
caseimportinfo-createcommand by allowing users to define the index case name.Bug Fixes