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

Define Pathname#as_json #25526

Merged
merged 1 commit into from
Jun 26, 2016
Merged

Conversation

tricknotes
Copy link
Contributor

Summary

When the Pathname object is converted as JSON,
it should be a string that means itself.

Expected:

>> Pathname.new('/path/to/somewhere.txt').as_json
"/path/to/somewhere.txt"

Actual:

>> Pathname.new('/path/to/somewhere.txt').as_json
{"path"=>"/path/to/somewhere.txt"}

When the Pathname object is converted as JSON,
it should be a string that means itself.

Expected:
```
>> Pathname.new('/path/to/somewhere.txt').as_json
"/path/to/somewhere.txt"
```

Actual:
```
>> Pathname.new('/path/to/somewhere.txt').as_json
{"path"=>"/path/to/somewhere.txt"}
```
@rails-bot
Copy link

r? @sgrif

(@rails-bot has picked a reviewer for you, use r? to override)

@sgrif
Copy link
Contributor

sgrif commented Jun 25, 2016

Thanks, but we aren't going to implement this for every type in the standard library without a compelling use case. This seems like a more appropriate patch for Ruby itself.

@sgrif sgrif closed this Jun 25, 2016
@chancancode chancancode reopened this Jun 26, 2016
@chancancode
Copy link
Member

I discussed this with @sgrif and I think we can probably merge this. While I agree it seems relatively unlikely to come up in practice, given the symmetry between URI and Pathname, it would feel a bit inconsistent if one works out-of-the-box and the other doesn't.

@chancancode chancancode merged commit f4c0dfb into rails:master Jun 26, 2016
@tricknotes tricknotes deleted the pathname-as-json branch June 26, 2016 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants