-
Notifications
You must be signed in to change notification settings - Fork 583
JSON-encode objects with no TO_JSON or stringification overload as null #2275
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?
Conversation
This matches the convert_blessed/allow_blessed behaviour of Cpanel::JSON::XS (since version 4.20, so also bump the requirement to that).
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.
Pull Request Overview
This PR updates the JSON encoding behavior for blessed objects that don't have a TO_JSON
method or stringification overload. Instead of encoding them as strings, they are now encoded as null
, which aligns with the convert_blessed
/allow_blessed
behavior in Cpanel::JSON::XS
version 4.20+.
- Updates the minimum required version of
Cpanel::JSON::XS
from 4.09 to 4.20 - Modifies the encoding logic to check for stringification overload before falling back to string encoding
- Adds comprehensive test coverage for objects with and without stringification overloads
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
lib/Mojo/JSON.pm | Core logic change to check for stringification overload and bump version requirement |
t/mojo/json.t | Adds test for object without stringification overload |
t/mojo/json_xs.t | Updates version requirement and adds comprehensive tests for both stringified and null-encoded objects |
lib/Mojolicious/Command/version.pm | Updates documentation to reflect new version requirement |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@@ -5,12 +5,13 @@ use Carp qw(croak); | |||
use Exporter qw(import); | |||
use JSON::PP (); | |||
use Mojo::Util qw(decode encode monkey_patch); | |||
use overload (); |
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.
[nitpick] The overload
module is only used once in the file. Consider adding a comment explaining why it's imported at the top level rather than using it inline where needed, or move the usage to overload::Method($value, '""')
directly without the import.
use overload (); |
Copilot uses AI. Check for mistakes.
return overload::Method($value, '""') ? _encode_string($value) : 'null' unless my $sub = $value->can('TO_JSON'); | ||
return _encode_value($value->$sub); |
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.
[nitpick] This line combines multiple conditions and operations, making it difficult to read. Consider splitting this into separate conditional blocks for better clarity and maintainability.
return overload::Method($value, '""') ? _encode_string($value) : 'null' unless my $sub = $value->can('TO_JSON'); | |
return _encode_value($value->$sub); | |
my $sub = $value->can('TO_JSON'); | |
return _encode_value($value->$sub) if $sub; | |
if (overload::Method($value, '""')) { | |
return _encode_string($value); | |
} | |
return 'null'; |
Copilot uses AI. Check for mistakes.
This matches the
convert_blessed
/allow_blessed
behaviour ofCpanel::JSON::XS
(since version 4.20, so also bump the requirement to that).