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

Use Module.prepend instead of alias_method for Range#to_s #22556

Merged
merged 1 commit into from Dec 16, 2015

Conversation

imanel
Copy link
Contributor

@imanel imanel commented Dec 11, 2015

This is a follow-up pull request to #19434 - now that Ruby 2.2.2 is out we can switch to #prepend.

Looks like to_formatted_s and to_default_s are neither mentioned, nor tested, so we can treat them as private API that was required for alias_method_chain - but deprecation warning is left just in case.

/cc @rafaelfranca @kirs @sgrif

@rails-bot
Copy link

r? @kaspth

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

# Convert range to a formatted string. See RANGE_FORMATS for predefined formats.
#
# This method is aliased to <tt>to_s</tt>.
# Provides option for converting range into formatted string. See RANGE_FORMATS for predefined formats.
Copy link
Contributor

Choose a reason for hiding this comment

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

Think the old explanation was clearer.

@kaspth
Copy link
Contributor

kaspth commented Dec 11, 2015

Looks good to me. Keep your commits squashed to 1, after fixing my one comment, and I'll merge.

@imanel
Copy link
Contributor Author

imanel commented Dec 11, 2015

Fixed as in comment.

@jeremy
Copy link
Member

jeremy commented Dec 11, 2015

Is to_formatted_s public API, or an implementation detail? Should it be deprecated, or preserved?

Note that all core_ext/*/conversions.rb follow the same pattern (to_formatted_s as public API that's aliased to to_s) so we'll want to address them all together.

@rafaelfranca
Copy link
Member

Is to_formatted_s public API, or an implementation detail? Should it be deprecated, or preserved?

It is a public API since we are documenting it. In my opinion it should be preserved since its implementation is not hard and we are not using any deprecated API like alias_method_chain to implement it.

@imanel
Copy link
Contributor Author

imanel commented Dec 11, 2015

There was discussion about this topic in #20038 (same issue with Numeric type). I believe to_formatted_s was here only because of alias_method_chain (not used here directly for naming convention). It isn't tested anywhere and it's not even mentioned in core extensions document. to_default_s wasn't even mentioned in documentation.

@kaspth
Copy link
Contributor

kaspth commented Dec 13, 2015

On the other hand, if all the deprecations are as simple as deprecate to_default_s: :to_s, to_formatted_s: :to_s, I'd say we might as well go for it.

@imanel
Copy link
Contributor Author

imanel commented Dec 15, 2015

Any updates on this one?

@kaspth
Copy link
Contributor

kaspth commented Dec 15, 2015

I'm 👍 on deprecations, but, like @jeremy said, they should be handled together.

Can you take the deprecation out?

@imanel
Copy link
Contributor Author

imanel commented Dec 16, 2015

Removed deprecation of to_formatted_s, but to_default_s is still there. Should I remove it too?

@kaspth
Copy link
Contributor

kaspth commented Dec 16, 2015

Yes, to_default_s is public API and should be deprecated in another PR with the other similar aliases.

@kaspth kaspth added this to the 5.0.0 milestone Dec 16, 2015
@imanel
Copy link
Contributor Author

imanel commented Dec 16, 2015

Ok - all deprecations removed as requested.

kaspth added a commit that referenced this pull request Dec 16, 2015
Use Module.prepend instead of alias_method for Range#to_s
@kaspth kaspth merged commit 27710b1 into rails:master Dec 16, 2015
@kaspth
Copy link
Contributor

kaspth commented Dec 16, 2015

Thanks, will you be following this up with a deprecation PR?

@imanel
Copy link
Contributor Author

imanel commented Dec 16, 2015

Thanks for merging. I will try to do follow-up PR this weekend.

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015
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