Add include_history parameter to Client search method #13
Conversation
WalkthroughAn optional Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #13 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 6
Lines 169 169
=========================================
Hits 169 169
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
quienesquien/client.py (1)
73-83: Document the newinclude_historyparameter.
The publicsearchdocstring omits the new argument (Lines 73-83). Add it to the Args list so callers understand the behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@README.md`:
- Line 126: The README currently states include_history default is False while
the method signature defaults to None; update documentation or code so they
agree: either change the method signature's default for the include_history
parameter to False in the function/method declaration that accepts
include_history, or explicitly document in README that include_history=None
means "omit from request / let API decide" and that False is the explicit
boolean default when set; reference the include_history parameter and the method
signature where it's declared when making the change.
There was a problem hiding this comment.
Pull request overview
This PR adds an include_history parameter to the Client's search method, allowing users to control whether historical records are included in search results. The version is bumped to 1.0.4, and the vcrpy test dependency is added to support test recording.
Changes:
- Added optional
include_historyboolean parameter to the search method - Updated README documentation to describe the new parameter
- Added vcrpy dependency for test fixtures
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| requirements-test.txt | Adds vcrpy==7.0.* dependency for test cassette recording |
| quienesquien/version.py | Bumps version from 1.0.3 to 1.0.4 |
| quienesquien/client.py | Adds include_history parameter to search method signature and includes it in the API request params |
| README.md | Documents the new include_history parameter in the Search Parameters section |
Comments suppressed due to low confidence (1)
quienesquien/client.py:87
- The docstring is missing documentation for the new
include_historyparameter. All other parameters are documented in the Args section, butinclude_historyis not mentioned. This should be added for consistency and to help users understand what this parameter does.
"""Perform a search request and return the results.
Args:
full_name (str): Full name of the person.
match_score (int): Minimum match percentage (default: 60).
rfc (str): Mexican RFC.
curp (str): Mexican CURP.
gender (Gender): masculino or femenino.
birthday (datetime.date): Date of birth.
search_type (SearchType): fisica or moral.
search_list (tuple[SearchList, ...]): Lists to search.
If not provided, searches all.
The search is hierarchical: it first looks for a match by RFC,
then by CURP if RFC is not found, and finally by name if
neither is found.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'birthday': birthday.strftime('%d/%m/%Y') if birthday else None, | ||
| 'type': search_type.value if search_type is not None else None, | ||
| 'list': ','.join(search_list) if search_list else None, | ||
| 'include_history': include_history, |
There was a problem hiding this comment.
When a Python boolean (True/False) is passed as a query parameter through httpx, it will be serialized as the capitalized string "True" or "False". Many REST APIs expect lowercase "true"/"false" or numeric "1"/"0" values for boolean parameters. You should verify that the API accepts "True"/"False" strings, or convert the boolean to the expected format. For example, you could use str(include_history).lower() if the API expects lowercase, or implement a converter function that maps True->1 and False->0 if the API expects numeric values.
| 'include_history': include_history, | |
| 'include_history': str(include_history).lower() if include_history is not None else None, |
| birthday: dt.date | None = None, | ||
| search_type: SearchType | None = None, | ||
| search_list: tuple[SearchList, ...] | None = None, | ||
| include_history: bool | None = None, |
There was a problem hiding this comment.
The codebase has comprehensive test coverage for the search method with various parameters (see tests/test_client.py lines 24-61), but no tests have been added for the new include_history parameter. Consider adding test cases that verify the behavior when include_history is set to True, False, and None to ensure the parameter is correctly passed to the API and behaves as expected.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.