using isoparse for less strict validations on date #2774
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a modification to date parsing in the Changes
Poem
Whispers (Because who doesn't love a slightly snarky poem about date parsing?) ✨ Finishing Touches
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/emr/resources/questionnaire/utils.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
care/emr/resources/questionnaire/utils.py (2)
69-69: Handle timezone information consistently.The
isoparsefunction will happily parse timezone information if present, but we're not explicitly handling it. This could lead to some... unexpected behavior when comparing dates.Consider either:
- Explicitly stripping timezone information if not needed
- Converting to UTC if timezone support is desired
- Rejecting timezone information if not supported
I can help implement any of these approaches if you'd like.
67-67: Consider validating date format strictness.While using
isoparsemakes the validation less strict (which I assume was the goal), it might be a bit too permissive. It would accept formats like "2024-1-1", "2024/01/01", or even "20240101", which could lead to... interesting situations.Consider adding a basic format check before parsing:
elif value_type == QuestionType.date.value: + if not re.match(r'^\d{4}-\d{2}-\d{2}', value.value): + raise ValueError("Date must start with YYYY-MM-DD format") isoparse(value.value).date() elif value_type == QuestionType.datetime.value: + if not re.match(r'^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}', value.value): + raise ValueError("Datetime must start with YYYY-MM-DDThh:mm:ss format") isoparse(value.value)Also, we should probably add some test cases:
Would you like me to generate test cases covering various date formats and edge cases?
Also applies to: 69-69
| @@ -2,6 +2,7 @@ | |||
| from datetime import datetime | |||
| from urllib.parse import urlparse | |||
|
|
|||
| from dateutil.parser import isoparse | |||
There was a problem hiding this comment.
💡 Codebase verification
Oh, looks like we're living dangerously without declaring python-dateutil anywhere
The dependency isn't listed in setup.py, pyproject.toml, or any requirements files (which seem to be... missing?). While python-dateutil might be installed in your environment, this could lead to some interesting surprises in production. You might want to:
- Add python-dateutil to setup.py's install_requires
- Or create a requirements.txt if that's more your style
🔗 Analysis chain
Verify dateutil dependency in requirements.
I see you've added dateutil, which is nice and all, but we should probably make sure it's properly listed in the project dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if python-dateutil is listed in requirements
fd -t f "requirements.*\.txt" -x grep -l "python-dateutil"
# Check if it's listed in setup.py or pyproject.toml
fd -t f "(setup\.py|pyproject\.toml)" -x grep -l "python-dateutil"
Length of output: 125
Script:
#!/bin/bash
# First, find all requirements files and setup configurations
echo "=== Finding requirement files ==="
fd -t f "requirements|setup\.py|pyproject"
echo -e "\n=== Checking content of found files ==="
# If files are found, check their content for dateutil
fd -t f "requirements|setup\.py|pyproject" -x cat {} \; -x echo -e "\n---\n" \;
Length of output: 3708
Script:
#!/bin/bash
# Check for requirements files in subdirectories
echo "=== Checking for requirements in subdirectories ==="
fd -t f "requirements.*\.(txt|in)" -d 3
# If found, examine their content
echo -e "\n=== Content of requirements files ==="
fd -t f "requirements.*\.(txt|in)" -d 3 -x cat {} \; -x echo -e "\n---\n" \;
Length of output: 328
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2774 +/- ##
========================================
Coverage 64.74% 64.75%
========================================
Files 252 252
Lines 12743 12744 +1
Branches 1119 1119
========================================
+ Hits 8251 8252 +1
Misses 4384 4384
Partials 108 108 ☔ View full report in Codecov by Sentry. |
vigneshhari
deleted the
prafful/bugs/making-date-validations-less-strict
branch
Proposed Changes
Associated Issue
Architecture changes
Merge Checklist
/docsOnly PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit