Fix guides for as core ext #6064

Merged
merged 3 commits into from May 7, 2012

Conversation

Projects
None yet
4 participants
Contributor

gazay commented Apr 29, 2012

Some fixes in guides for deprecated information and tests for deprecated information about Range#===

/cc @fxn

Member

steveklabnik commented Apr 29, 2012

Guides are supposed to be merged into lifo/docrails.

Contributor

gazay commented Apr 29, 2012

@steveklabnik yep, I did it there, but @fxn asked me to pull it here - https://github.com/lifo/docrails/pull/94

Member

steveklabnik commented Apr 29, 2012

Word. My bad!

Member

steveklabnik commented Apr 29, 2012

Ahh, okay. yeah, so after you get your first commit, you get access to push to docrails, and then they go there afterwards.

Owner

fxn commented Apr 29, 2012

Actually, docrails has public write access.

This was sent to docrails as a pull request. It is good that is a pull request because a double check will be healthy for this particular edit.

The thing is docrails is not a project, and we do not do pull requests in docrails. docrails is only meant for doc commits people are confident to push themselves. Usually short and non-controversial (see http://weblog.rubyonrails.org/2012/3/7/what-is-docrails/).

So, I asked @gazay to do the pull request here and /cc me.

Member

steveklabnik commented Apr 29, 2012

I'll just shut up now. :)

Owner

fxn commented Apr 29, 2012

@steveklabnik haha :)

Member

steveklabnik commented Apr 29, 2012

❤️

Contributor

gazay commented Apr 30, 2012

Btw, I think it can be cherry-picked into 3.2.3 stable branch with my previous commit about String#truncate

@vijaydev vijaydev commented on the diff May 1, 2012

guides/source/active_support_core_extensions.textile
@@ -2103,20 +2097,20 @@ To do so it sends +to_xml+ to every item in turn, and collects the results under
By default, the name of the root element is the underscorized and dasherized plural of the name of the class of the first item, provided the rest of elements belong to that type (checked with <tt>is_a?</tt>) and they are not hashes. In the example above that's "contributors".
-If there's any element that does not belong to the type of the first one the root node becomes "records":
+If there's any element that does not belong to the type of the first one the root node becomes "objects":
@vijaydev

vijaydev May 1, 2012

Member

Is there a reason behind this change?

@gazay

gazay May 1, 2012

Contributor

The code for it was changed long time ago - about 2 years 580dd3b#L2L134
I just noticed that guides for this code are deprecated and developer can't get what he expected after reading this line

@vijaydev

vijaydev May 1, 2012

Member

Got it. Thanks.

@vijaydev vijaydev and 1 other commented on an outdated diff May 2, 2012

guides/source/active_support_core_extensions.textile
@@ -2722,8 +2716,6 @@ Active Support extends this method so that the argument may be another range in
(1...9).include?(3..9) # => false
</ruby>
-WARNING: The original +Range#include?+ is still the one aliased to +Range#===+.
@vijaydev

vijaydev May 2, 2012

Member

Has this changed recently?

@gazay

gazay May 3, 2012

Contributor

This was changed in ruby. In versions >1.9 Range#=== is just an alias of Range#include?
Maybe there is a point to do new commit with alias_method :=== :include_without_range? instead of changing guides for compatibility with previous usage of Range?

Owner

fxn commented May 7, 2012

This pull request touches too many things.

  • 👍 to removing the docs for those class_inheritable_* methods.
  • The documentation of truncate has actually to say that both strings and regexps are valid arguments. It has to say that in English, not implicitly via an example. Examples then could depict each use case.
  • The docs for the time extensions to Numeric are good, but instead of Time.now the equivalence is with Time.current (and the API is wrong here also if I am not mistaken).
  • The comment about Range#include? means that albeit === is an alias of the original method, you don't get these extensions on === (because aliasing is not transitive). So I think the warning should be left as is.

If you revise those it may be good to go. We may copy edit a little bit perhaps later, but no big deal.

Contributor

gazay commented May 7, 2012

@fxn I rewrote examples for String#truncate

About Time.now and Time.current - you right, if there set time zone, Time.now.advance will be different with time extensions for Numeric, because they use ::Time.current which returns TimeWithZone object. I rewrote calls to Time.new to Time.current in guides and Numeric docs.

About wrong API - I just took these docs from Numeric extensions file: https://github.com/rails/rails/blob/master/activesupport/lib/active_support/core_ext/numeric/time.rb#L6 and checked before making this pull request. Also tests passed with this API.

About Range#=== I created gist which explains why this warning should be deleted or new alias should be added as I wrote to @vijaydev: https://gist.github.com/2629683

Owner

fxn commented May 7, 2012

@gazay gotcha.

Then I think we should document that === is also extended. Both methods are related, and the reader wonders how touching one affects the other one.

Owner

fxn commented May 7, 2012

Ah, regarding Time.current almost any occurrence of Time.now in the docs is a smell. You almost always want Time.current when working with Rails as a user, and Active Support implements all date and time methods based on Time.current. The application is the one in charge of choosing the time zone, not the machine's clock.

Contributor

gazay commented May 7, 2012

@fxn I wrote description for Range#=== gazay/rails@66e0e42#L0L2710

and in one more place changed Time.now to Time.current in guides for AS core_ext

Owner

fxn commented May 7, 2012

Awesome, thanks for working on this!

fxn merged commit ed2feb7 into rails:master May 7, 2012

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