-
Notifications
You must be signed in to change notification settings - Fork 80
refactor: Simplified type-conforming logic #2964
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
refactor: Simplified type-conforming logic #2964
Conversation
Reviewer's Guide by SourceryThe pull request refactors the Sequence diagram for _conform_primitive_property functionsequenceDiagram
participant Caller
participant _conform_primitive_property
participant _handle_datetime
participant _handle_date
participant _handle_timedelta
participant _handle_time
participant _handle_bytes
participant _handle_numeric
Caller->>_conform_primitive_property: _conform_primitive_property(elem, property_schema)
alt elem is datetime.datetime
_conform_primitive_property->>_handle_datetime: _handle_datetime(elem, property_schema)
_handle_datetime-->>_conform_primitive_property: return converted elem
else elem is datetime.date
_conform_primitive_property->>_handle_date: _handle_date(elem, property_schema)
_handle_date-->>_conform_primitive_property: return converted elem
else elem is datetime.timedelta
_conform_primitive_property->>_handle_timedelta: _handle_timedelta(elem, property_schema)
_handle_timedelta-->>_conform_primitive_property: return converted elem
else elem is datetime.time
_conform_primitive_property->>_handle_time: _handle_time(elem, property_schema)
_handle_time-->>_conform_primitive_property: return converted elem
else elem is bytes
_conform_primitive_property->>_handle_bytes: _handle_bytes(elem, property_schema)
_handle_bytes-->>_conform_primitive_property: return converted elem
else elem is float or decimal.Decimal
_conform_primitive_property->>_handle_numeric: _handle_numeric(elem, property_schema)
_handle_numeric-->>_conform_primitive_property: return converted elem
else elem is boolean
_conform_primitive_property->>_conform_primitive_property: elem != 0
_conform_primitive_property-->>Caller: return converted elem
else
_conform_primitive_property-->>Caller: return elem
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2964 +/- ##
==========================================
+ Coverage 93.47% 93.51% +0.03%
==========================================
Files 69 69
Lines 5658 5658
Branches 699 693 -6
==========================================
+ Hits 5289 5291 +2
+ Misses 264 263 -1
+ Partials 105 104 -1
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:
|
CodSpeed Performance ReportMerging #2964 will degrade performances by 22.13%Comparing Summary
Benchmarks breakdown
|
|
@sourcery-ai review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @edgarrmondragon - I've reviewed your changes - here's some feedback:
Overall Comments:
- The new structure is more readable, but consider if the handler functions could be simplified further, perhaps by sharing common logic.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @edgarrmondragon - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding type hints to the
elemargument in the handler functions for better clarity. - The
_handle_datetimefunction doesn't need theproperty_schemaargument, so you can remove it.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Summary by Sourcery
Refactor the type-conforming logic in the _typing module to improve code structure and readability
Enhancements: