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

fixes Fixnum deprecation warning for ruby 2.4.0 #364

Closed
wants to merge 29 commits into from

Conversation

hwhelchel
Copy link

@hwhelchel hwhelchel commented Dec 29, 2016

@seejohnrun thanks for a great gem! Would love to add this support for ruby 2.4.0. Let me know if you need any thing else to accept this PR.

Thanks!

@hwhelchel
Copy link
Author

@seejohnrun looks like the build failed on 2.1 and lower because it's pulling in activesupport-5-0-1 which requires ruby 2.2.2 or higher. Ruby 2.4.0 build failed because rvm couldn't find the ruby version.

@seejohnrun
Copy link
Collaborator

@hwhelchel hmm - I wonder if we should have a few test files that pull in specific versions? Otherwise we'd need to remove those tests and change the support matrix which I'm not sure I'm ready to do (a lot of people unfortunately still on ruby <2 last I checked)

@hwhelchel
Copy link
Author

Happy to help fix any issues. I don't understand what is wrong. Looking at the travis 1.9.3 build, the error is

activesupport-5.0.1 requires ruby version >= 2.2.2, which is incompatible with

Do you know why it is trying to pull in the latest version of Active Support?

screen shot 2017-01-02 at 2 14 52 pm

@hwhelchel
Copy link
Author

Hmm I can't quite get all 5 builds to pass. Do we need separate gemspecs for each ruby version?

@seejohnrun
Copy link
Collaborator

I don't think so as long as the Gemfiles don't specify versions.
I am going to take a look at this tonight hopefully and try to see if I can help get this going

@@ -0,0 +1,9 @@
module IntegerUtil
def self.klass
if RUBY_VERSION.include?("2.4.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this probably should be something more concrete / robust - given versions after 2.4 will also have this change. Helpful to look @ rails/rails#25056

@@ -1,6 +1,6 @@
script: "bundle exec rspec spec"
before_install:
- gem install bundler
- gem install bundler && gem update bundler
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this line?

@@ -127,8 +127,8 @@ def self.end_of_date(date, reference=Time.now)
def self.sym_to_month(sym)
MONTHS.fetch(sym) do |k|
MONTHS.values.detect { |i| i.to_s == k.to_s } or
raise ArgumentError, "Expecting Fixnum or Symbol value for month. " \
"No such month: #{k.inspect}"
raise ArgumentError, "Expecting #{IntegerUtil.klass} or "\
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're going to change the indentation here, can we just move to one line?

@seejohnrun
Copy link
Collaborator

Maybe something like s.add_development_dependency('activesupport', ['>= 3.0.0', ('<5' if Gem::Version.new(RUBY_VERSION.dup) < Gem::Version.new('2.2.2'))].compact) ?

I'm looking for some prior art on how others are handling this but nothing yet

Don't try to bring in new activesupport on versions of Ruby that don't
support it
@seejohnrun
Copy link
Collaborator

FYI I'm testing this idea separately here: #369

seejohnrun and others added 10 commits January 13, 2017 09:12
Also modify `Rule#from_ical` to use `Rule#from_hash` for uniformity and
centralization of parsing logic.
[TYPO] Fixes a couple of typos in the README.md
…le-name

Fix incorrect variable name in previous_occurrences.
@avit
Copy link
Collaborator

avit commented Jan 19, 2017

@hwhelchel Thanks for the contribution. This has a lot of unrelated changes included, could you please rebase and resubmit a clean branch?

I'm also thinking if we could make this class reference static instead of requiring a method call, something like this:

module IceCube
  unless Object.const_defined?(:Integer) && Integer == 1.class
    Integer = Fixnum
  end
end
#=> IceCube::Integer

Then all references to Fixnum could just be changed to Integer and it would find the one from our module if it's defined there.

@hwhelchel
Copy link
Author

@avit sounds good. I'll take a look at cleaning this up.

@patrickjaberg
Copy link
Contributor

@seejohnrun FWIW, it looks like Fixnum is used as a way of validating that arguments are integers. As such, it seems that replacing Fixnum references with Integer may be all that's required. Integer has been around since at least 1.8.7 (I didn't look back further), so I'd think it would be sufficiently backwards compatible for general use. See patrickjaberg@489808f for an example against v0.15.0

I'd be happy to submit a PR if you're interested. Thanks!

@seejohnrun
Copy link
Collaborator

Yep I'm down for this for sure @patrickjaberg

@avit
Copy link
Collaborator

avit commented Mar 19, 2017

Merged #379 instead. Thanks for the contribution.

@avit avit closed this Mar 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants