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.

Sign up for GitHub

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

Jump to bottom

[#2867] modified Date#>> to take calendar reforms under consideration #3091

Closed
wants to merge 6 commits into from
Closed

[#2867] modified Date#>> to take calendar reforms under consideration #3091

wants to merge 6 commits into from
+41 −2

Conversation

ranslogs
Copy link
Contributor

[jruby#2867] modified Date#>> to take calendar reforms under consider…
c4e20e4 
…ation

- added tests for next month with calendar reform
- uses solution provided in jruby#1769 comments
- also fixes issues jruby#1769
@kares
Copy link
Member

kares commented Jun 29, 2015

@raeoks would you mind trying to target jruby-1_7 so that this is fixed in JRuby 1.7.x as well?
... don't think test should be added under test/mri as those might conflict when the MRI suite is merged

@eregon since its mostly your work-around ... any thoughts on getting those in?

@eregon
Copy link
Member

eregon commented Jun 29, 2015

These tests would be useful in RubySpec. You can just do a separate commit touching spec/ruby.

Well, if this behavior must be supported, I'd say there is not much choice.

@ranslogs
Copy link
Contributor Author

I'll move test cases from test/mri/date/test_date_arith.rb to spec/ruby/library/date's add_month_spec.rb and next_year_spec.rb files.
Also I'll be start working on creating a new pull request to fix same issue on jruby-1_7.

@headius
Copy link
Member

headius commented Jul 7, 2015

The changes look ok to me but I defer to @eregon on when and whether this should merge.

@eregon
Copy link
Member

eregon commented Jul 8, 2015

@raeoks Could you move the test cases to specs? Or should I do it?

@ranslogs
Copy link
Contributor Author

ranslogs commented Jul 8, 2015

@eregon I'll do it and doing it right now.

@ranslogs
Copy link
Contributor Author

@eregon Should I create a new pull request resolving #2867 for Jruby 1.7?

@eregon
Copy link
Member

eregon commented Jul 20, 2015

I guess yes but better ask @kares.
@raeoks Is this PR ready to be merged?

@kares
Copy link
Member

kares commented Jul 21, 2015

optimally, there would be only one PR targeting jruby-1_7 (branch is periodically merged to master) ...

@eregon
Copy link
Member

eregon commented Jul 21, 2015

I applied the commits to 1.7, 1.9-mode (1.8 Date is just so different and should not be affected the same way by this bug): 40e9140 and ec80a32.

I guess the merge in this case would not work so well (date.rb moved quite a bit), so I'd rather merge #3157 in master instead.

@eregon eregon closed this Jul 21, 2015
@kares kares added this to the JRuby 1.7.22 milestone Jul 21, 2015
@eregon eregon mentioned this pull request Jul 23, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
stdlib
Projects
None yet
Milestone
JRuby 1.7.22
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ranslogs @kares @eregon @headius