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

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

Closed
wants to merge 6 commits into from

Conversation

Projects
None yet
4 participants
@raeoks
Copy link
Contributor

raeoks commented Jun 29, 2015

  • added tests for next month with calendar reform
  • uses solution provided in #1769 comments
  • also fixes issues #1769
Ranjeet Singh
[#2867] modified Date#>> to take calendar reforms under consideration
- added tests for next month with calendar reform
- uses solution provided in #1769 comments
- also fixes issues #1769
@kares

This comment has been minimized.

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

This comment has been minimized.

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.

@raeoks

This comment has been minimized.

Copy link
Contributor Author

raeoks commented Jun 30, 2015

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

eregon commented Jul 8, 2015

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

@raeoks

This comment has been minimized.

Copy link
Contributor Author

raeoks commented Jul 8, 2015

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

@raeoks

This comment has been minimized.

Copy link
Contributor Author

raeoks commented Jul 20, 2015

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

@eregon

This comment has been minimized.

Copy link
Member

eregon commented Jul 20, 2015

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

@kares

This comment has been minimized.

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

This comment has been minimized.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.