Skip to content
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

Add IPAddr#to_json/as_json #77

Merged
merged 1 commit into from
Oct 18, 2024
Merged

Add IPAddr#to_json/as_json #77

merged 1 commit into from
Oct 18, 2024

Conversation

taketo1113
Copy link
Contributor

Add to_json/as_json methods to get json format string.

When use json gem, IPAddr#to_json remove prefix address with CIDR.
Although json gem have no paln to support for IPAddr class.
ruby/json#503

Actual behavior

require 'ipaddr'
require 'json'

IPAddr.new('172.16.0.0/24').to_json #=> "\"172.16.0.0\""

This PR behavior

IPAddr#to_json method return json format address wit prefix without json gem.
(When to require json gem, it have same behavior.)

require 'ipaddr'

IPAddr.new('172.16.0.0/24').to_json #=> "\"172.16.0.0/24\""

IPAddr.new('172.16.0.1').to_json #=> "\"172.16.0.1\""


# Returns a json string containing the IP address representation.
def to_json(*args)
format("\"%s\"", as_json)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was your intent as_json(*args) because you gave it a name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@knu It is not necessary a name. I have removed a name of args.
It is from another patch which is ruby/json@f1ffc4a

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the implementation of to_json to be as_json.to_json(*args) so it is easier to change how IPAddrs are encoded simply by overriding as_json. With the current definition, when you want it to be def as_json(*) = to_f you'll need to rewrite to_json as well.

Updated to use cidr method when return address with prefix in #as_json
Copy link
Member

@knu knu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forget about to_f, I think it mistook this for something else. In any case, I like how JSON::GenericObject defines to_json. I'll change it as appropriate later. Thanks for contributing!

@knu knu merged commit 2ea26e4 into ruby:master Oct 18, 2024
27 checks passed
@knu
Copy link
Member

knu commented Oct 18, 2024

The json library defines the generic to_json in Object, so we may not have to implement one in IPAddr.
Should we provide to_json so you can use it without the json library?

@taketo1113
Copy link
Contributor Author

@knu

The json library defines the generic to_json in Object, so we may not have to implement one in IPAddr. Should we provide to_json so you can use it without the json library?

Because when use to_json method of json gem, return json format string without prefix.
It is unable to serialize/deserialize IPAddr/CIDR value.

Expected behavior

require 'ipaddr'
require 'json'

IPAddr.new('172.16.0.0/24').to_json #=> "\"172.16.0.0/24\""

Actual behavior

require 'ipaddr'
require 'json'

IPAddr.new('172.16.0.0/24').to_json #=> "\"172.16.0.0\""

ruby/json#503

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants