-
Notifications
You must be signed in to change notification settings - Fork 184
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
Implement beginless range #2155
Conversation
598038c
to
1d5401d
Compare
@norswap Could you review this one? Looks good to me :) |
Is |
@LillianZ Do you mean the Eclipse error where it can't find
The problem has been known for some time, but it's in limbo as it requires digging into the OpenJDK build process. It's fixed on the GraalVM image builds however. |
1d5401d
to
70e1c24
Compare
Got it, thank you! I misunderstood the error logs. Force-pushed some spacing for lint. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me so far!
Another place I remember I had to handle endless ranges explicitly was StringNodes#GetIndexNode
.
b22b033
to
304493f
Compare
Hello LillianZ, thanks for contributing a PR to our project! We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address LillianZ -(at)- users -(dot)- noreply -(dot)- github -(dot)- com. You can sign it at that link. If you think you've already signed it, please comment below and we'll check. |
@LillianZ I can add that email to the list, but it would be better to rebase instead of merge to have a clean history. |
LillianZ has signed the Oracle Contributor Agreement (based on email address LillianZ -(at)- users -(dot)- noreply -(dot)- github -(dot)- com) so can contribute to this repository. |
Sorry, a conflict appeared online not appearing locally, and trying to resolve it caused a merge. I have been trying to rebase. |
FWIW, the GitHub UI often thinks there are conflicts in CHANGELOG.md, but actually there isn't due to the union merge strategy which never conflicts. Best is to just ignore those, because there are no actual conflicts. |
c83d659
to
2dfc5a2
Compare
To confirm: it's okay ignore when GitHub UI complains about a conflict (at least in CHANGELOG) because it's something you can fix? |
and internal beginless? predicate
920f5ab
to
d81bbb9
Compare
I am slightly concerned that 2.6 specs have been running for 3 hours, but they passed locally so I'm not sure what to do. |
Looks broken - it's not running at all. CI is hard. |
3b0b2ab
to
473bc1a
Compare
Thanks for checking! I fixed my commit message to get CI to rerun. |
Most likely this indicates a spec getting stuck when run on MRI 2.6.6.
So I'll add the |
Looks like it's generating some kind of infinite string or something. We're investigating. |
It's fixed! For some reason I didn't realize the specs breaking on CRuby 2.6 => fix 2.6 specs. |
spec/ruby/core/array/fill_spec.rb
Outdated
ruby_version_is "2.7" do | ||
it "works with beginless ranges" do | ||
[1, 2, 3, 4].fill('x', eval("(nil..2)")).should == ["x", "x", "x", 4] | ||
[1, 2, 3, 4].fill(eval("(nil...2)")) { |x| x + 2 }.should == [2, 3, 3, 4] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove the extra nil
s here? Also happens in other specs.
Failures:
|
83a9e9f
to
c3cdb10
Compare
c3cdb10
to
80ff0a2
Compare
Woot! everything's finally passing! sorry for the delay, and thanks for all the tips; I found the range#max patch in ruby-lang and definitely would not have known to go looking in github! |
PullRequest: truffleruby/2226
Merged! Congrats 👍 |
Starting to implement beginless ranges for Ruby 2.7 compatibility #2004
So far:
Implemented or added specs for Range#{new, bsearch, count, each, equal_value, first, inspect, max, min, size} regarding beginless range compatibility
Implemented or added specs for Array#{[], []=, slice, slice!, to_a, fill, values_at} regarding beginless range compatibility
To do:
Range#cover?/include/===
some or all of Range#{entires, eql?, hash, last, member?, minmax, step/%, to_s}
Methods outside of Array and Range, such as String methods.
Shopify#1