-
Notifications
You must be signed in to change notification settings - Fork 190
fix (python): skip passing None to non nullable params #1245
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
Conversation
WalkthroughA new Twig filter Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
e1af9f4 to
42df221
Compare
fcc7179 to
9cab054
Compare
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
templates/python/base/params.twig (1)
24-24: Add defensive check for empty consumes array.Accessing
method.consumes[0]without verifying the array is non-empty could cause a template error if a method has no content-type specified.Apply this diff to add a safety check:
-{% set isMultipart = method.consumes[0] == "multipart/form-data" %} +{% set isMultipart = method.consumes|length > 0 and method.consumes[0] == "multipart/form-data" %}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/SDK/Language/Python.php(1 hunks)templates/php/docs/service.md.twig(0 hunks)templates/python/base/params.twig(1 hunks)
💤 Files with no reviewable changes (1)
- templates/php/docs/service.md.twig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: web (client)
- GitHub Check: dart (server)
- GitHub Check: apple (client)
- GitHub Check: node (server)
- GitHub Check: kotlin (server)
- GitHub Check: android (client)
- GitHub Check: build (8.3, Python310)
- GitHub Check: build (8.3, Go118)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, CLINode18)
- GitHub Check: build (8.3, DotNet80)
- GitHub Check: build (8.3, CLINode16)
- GitHub Check: build (8.3, DartBeta)
- GitHub Check: build (8.3, Android5Java17)
🔇 Additional comments (3)
templates/python/base/params.twig (2)
15-20: Good addition to prevent None values for non-nullable parameters.The conditional logic correctly ensures that optional non-nullable query parameters are only included when they have a value, preventing
Nonefrom being passed to endpoints that don't accept null values.
22-32: Unified body parameter handling looks good.The refactored body parameter handling successfully applies the same None-skipping logic as query parameters, ensuring consistency across the codebase. The integration with
formatParamValueproperly handles multipart form data formatting while preserving the None-check behavior for optional non-nullable parameters.src/SDK/Language/Python.php (1)
402-407: Consider handling additional types for multipart form data.The filter currently only converts booleans to lowercase strings, leaving integers, floats, and other types unchanged. In multipart/form-data requests, most HTTP libraries expect all field values to be strings. Consider whether numeric types should also be explicitly converted to strings.
Additionally, when a nullable parameter has a
Nonevalue, the generated expressionstr(None).lower() if type(None) is bool else Nonewill evaluate toNone, which may then be serialized as the string"None"by the HTTP library rather than being omitted. Verify whether this behavior aligns with the API's expectations for nullable parameters.
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/SDK/Language/Python.php (1)
402-407: Consider using class constants for type comparisons.The filter uses string literals
'string'and'array'for type checking, but the class defines constants likeself::TYPE_STRINGandself::TYPE_ARRAY. For consistency and maintainability, consider using these constants instead.Apply this diff:
- new TwigFilter('formatParamValue', function (string $paramName, string $paramType, bool $isMultipartFormData) { - if ($isMultipartFormData && $paramType !== 'string' && $paramType !== 'array') { + new TwigFilter('formatParamValue', function (string $paramName, string $paramType, bool $isMultipartFormData) { + if ($isMultipartFormData && $paramType !== self::TYPE_STRING && $paramType !== self::TYPE_ARRAY) { return "str({$paramName}).lower() if type({$paramName}) is bool else {$paramName}"; } return $paramName;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/SDK/Language/Python.php(1 hunks)templates/python/base/params.twig(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, KotlinJava8)
- GitHub Check: build (8.3, KotlinJava11)
🔇 Additional comments (2)
templates/python/base/params.twig (2)
15-20: Good implementation of None-checking for query parameters.The conditional logic correctly skips adding query parameters to the API params dictionary when they are non-nullable, non-required, and have a None value. This aligns with the PR objective.
26-31: Correct implementation of None-checking for body/formData parameters.The conditional logic properly handles non-nullable, non-required parameters by checking for None before adding them to
api_params. The use offormattedValueensures proper formatting for multipart form data scenarios. This successfully achieves the PR objective.
9cab054 to
7a8cdde
Compare
Summary by CodeRabbit