🛠️ Refactor: SubjectsEndpoint - DRY filtering logic#623
Conversation
Collapsed duplicated site filtering logic in `SubjectsEndpoint` into a shared `_filter_by_site` method. This improves maintainability and ensures consistent behavior between synchronous and asynchronous operations. Verified with new unit tests covering both execution paths. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Collapsed duplicated site filtering logic in `SubjectsEndpoint` into a shared `_filter_by_site` method. This improves maintainability and ensures consistent behavior between synchronous and asynchronous operations. Verified with new unit tests covering both execution paths. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Collapsed duplicated site filtering logic in `SubjectsEndpoint` into a shared `_filter_by_site` method. This improves maintainability and ensures consistent behavior between synchronous and asynchronous operations. Verified with new unit tests covering both execution paths and adhered to code quality standards (formatting/linting). Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
fderuiter
deleted the
refactor-subject-filtering-dry-8831546442608404248
branch
There was a problem hiding this comment.
Pull request overview
This PR refactors the SubjectsEndpoint class to eliminate code duplication by extracting the site filtering logic into a reusable private method _filter_by_site. The refactoring follows the DRY (Don't Repeat Yourself) principle and maintains backward compatibility.
Changes:
- Extracted duplicated filtering logic into
_filter_by_sitehelper method - Refactored
list_by_siteandasync_list_by_siteto use the shared helper - Added comprehensive regression tests for both sync and async filtering
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
imednet/endpoints/subjects.py |
Extracted _filter_by_site method to eliminate duplicated filtering logic between sync and async methods |
tests/unit/endpoints/test_subjects_filtering.py |
Added regression tests covering both sync and async filtering with int/str parameters and edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Subject(studyKey="sk", subjectId=1, siteId=101, subjectKey="s1"), | ||
| Subject(studyKey="sk", subjectId=2, siteId=102, subjectKey="s2"), | ||
| Subject(studyKey="sk", subjectId=3, siteId=101, subjectKey="s3"), | ||
| Subject(studyKey="sk", subjectId=4, siteId="101", subjectKey="s4"), # String siteId |
There was a problem hiding this comment.
The comment "String siteId" is slightly misleading. While the Subject is constructed with a string siteId value, Pydantic's normalization will convert it to an integer (via parse_int_or_default). The actual test coverage is for filtering when the site_id parameter is passed as either int or str, which is tested on lines 29-30 and 53-54. Consider updating the comment to clarify this, or remove it if it's not necessary.
| Subject(studyKey="sk", subjectId=4, siteId="101", subjectKey="s4"), # String siteId | |
| Subject(studyKey="sk", subjectId=4, siteId="101", subjectKey="s4"), # siteId passed as string (normalized to int by Pydantic) |
Extracted duplicated filtering logic in
imednet/endpoints/subjects.pyinto_filter_by_site. Added regression tests intests/unit/endpoints/test_subjects_filtering.py.PR created automatically by Jules for task 8831546442608404248 started by @fderuiter