Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Tiny improvements of Range for X19 #2272

Closed
wants to merge 2 commits into from

2 participants

@kachick
Collaborator

No description provided.

@dbussink dbussink commented on the diff
kernel/common/range19.rb
@@ -1,6 +1,7 @@
# -*- encoding: us-ascii -*-
class Range
+ # Don't use `alias_method` for #===. `Delegate to #include?` is a spec.
@dbussink Owner

If this is defined as the spec, we should also add a RubySpec for this behavior

@kachick Collaborator
kachick added a note

Sorry spec is range/case_compare_spec.
But I want to this comment to here for readability.

@dbussink Owner

Oh, sorry from my side, I didn't realize it was just adding this as a comment and not changing the actual code. Were you surprised by this behavior? Behavior like this exists is various other places in Ruby and we don't have comments in all the other places. The upside of actually having a spec for this, is that even if people would think about changing it, the spec would fail and would tell people exactly what is wrong. So I don't really see a big advantage of adding this comment here.

@kachick Collaborator
kachick added a note

Yes, I was very surprised this behavior :)
And i was helped by a coment from non updated version guard in spec

But this is my problem, you are right !

@dbussink Owner

Yeah, adding comments like this is often a balancing act. Since the case of a certain being implement with another and not an alias happens in a few more places, it's not that much an edge case as probably the case you mentioned. I remember fixing 17e1e62, but didn't add a comment for that case either.

@kachick Collaborator
kachick added a note

Great reference! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbussink dbussink commented on the diff
kernel/common/range19.rb
@@ -83,6 +84,6 @@ def min(&block)
protected
def can_iterate_from?(object)
- first.respond_to?(:succ) && !object.kind_of?(Time)
+ object.respond_to?(:succ) && !object.kind_of?(Time)
@dbussink Owner

Same here, if this fixes a bug, we should also add a spec for it

@kachick Collaborator
kachick added a note

OK.
But that is difficult for me.
I remove this commit.

@dbussink Owner

Well, if this is a bug, it would be good to fix it. Do you have a script perhaps that shows the problem? We can work on making a proper spec out of that then.

@kachick Collaborator
kachick added a note

Thanks!

But i have no example for this commit.
This commit from Range#each already gotten the first element.
And this method is not used by other objects.
Prefer private to protected for me.

@dbussink Owner

We can make it private yeah, seems better to me then. Also inside the kernel/ code we prefer to use explicitly making things private, so basically doing private :method_name after the definition, instead of changing the default visibility by just a plain 'private'. We should probably apply this change for visibility to both 1.8 and 1.9 mode.

@kachick Collaborator
kachick added a note

I'll commit it via other pull-request without "first -> object" modifying.

@dbussink Owner

I'm fine with the first -> object change, seems to make sense to me, even though it will hardly ever be a problem in practice.

@kachick Collaborator
kachick added a note

I've got it. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kachick kachick closed this
@kachick kachick referenced this pull request from a commit in kachick/rubinius
@kachick kachick Fix an inner method Range#can_iterate_from?
Prefer protected to private
--------------------------------------

* This method is just used by `self.each`.

Replace receiver for X19
------------------------

* `first` is gotten the `self.each`

refer
-----

  * rubinius#2272
  * #2272
52d471a
@kachick kachick referenced this pull request from a commit in kachick/rubinius
@kachick kachick Fix an inner method Range#can_iterate_from?
Prefer protected to private
--------------------------------------

* This method is just used by `self.each`.

Replace receiver for X19
------------------------

* `first` is gotten the `self.each`

refer
-----

  * rubinius#2272 (diff)
  * #2272
5055f4a
@kachick kachick referenced this pull request from a commit in kachick/rubinius
@kachick kachick Fix an inner method Range#can_iterate_from?
Prefer protected to private
--------------------------------------

* This method is just used by `self.each`.

Replace receiver for X19
------------------------

* `first` is gotten the `self.each`

refer
-----

  * rubinius#2272 (diff)
  * #2272
71d1418
@kachick kachick deleted the kachick:improve-range-inner_code branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 2 additions and 1 deletion.
  1. +2 −1  kernel/common/range19.rb
View
3  kernel/common/range19.rb
@@ -1,6 +1,7 @@
# -*- encoding: us-ascii -*-
class Range
+ # Don't use `alias_method` for #===. `Delegate to #include?` is a spec.
@dbussink Owner

If this is defined as the spec, we should also add a RubySpec for this behavior

@kachick Collaborator
kachick added a note

Sorry spec is range/case_compare_spec.
But I want to this comment to here for readability.

@dbussink Owner

Oh, sorry from my side, I didn't realize it was just adding this as a comment and not changing the actual code. Were you surprised by this behavior? Behavior like this exists is various other places in Ruby and we don't have comments in all the other places. The upside of actually having a spec for this, is that even if people would think about changing it, the spec would fail and would tell people exactly what is wrong. So I don't really see a big advantage of adding this comment here.

@kachick Collaborator
kachick added a note

Yes, I was very surprised this behavior :)
And i was helped by a coment from non updated version guard in spec

But this is my problem, you are right !

@dbussink Owner

Yeah, adding comments like this is often a balancing act. Since the case of a certain being implement with another and not an alias happens in a few more places, it's not that much an edge case as probably the case you mentioned. I remember fixing 17e1e62, but didn't add a comment for that case either.

@kachick Collaborator
kachick added a note

Great reference! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
def ===(value)
include?(value)
end
@@ -83,6 +84,6 @@ def min(&block)
protected
def can_iterate_from?(object)
- first.respond_to?(:succ) && !object.kind_of?(Time)
+ object.respond_to?(:succ) && !object.kind_of?(Time)
@dbussink Owner

Same here, if this fixes a bug, we should also add a spec for it

@kachick Collaborator
kachick added a note

OK.
But that is difficult for me.
I remove this commit.

@dbussink Owner

Well, if this is a bug, it would be good to fix it. Do you have a script perhaps that shows the problem? We can work on making a proper spec out of that then.

@kachick Collaborator
kachick added a note

Thanks!

But i have no example for this commit.
This commit from Range#each already gotten the first element.
And this method is not used by other objects.
Prefer private to protected for me.

@dbussink Owner

We can make it private yeah, seems better to me then. Also inside the kernel/ code we prefer to use explicitly making things private, so basically doing private :method_name after the definition, instead of changing the default visibility by just a plain 'private'. We should probably apply this change for visibility to both 1.8 and 1.9 mode.

@kachick Collaborator
kachick added a note

I'll commit it via other pull-request without "first -> object" modifying.

@dbussink Owner

I'm fine with the first -> object change, seems to make sense to me, even though it will hardly ever be a problem in practice.

@kachick Collaborator
kachick added a note

I've got it. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
end
Something went wrong with that request. Please try again.