search

bokmann / business_time

Support for doing time math in business hours and days
MIT License
1.27k stars 215 forks source link

Ruby 2.4 resolved deprecation of Fixnum #148

Closed tom-lord closed 7 years ago

tom-lord commented 7 years ago

Replace references to Fixnum with Integer, to fix the existing deprecation warning:

business_time-0.7.6/lib/business_time/core_ext/fixnum.rb:6: warning: constant ::Fixnum is deprecated

tom-lord commented 7 years ago

Actually hang on.. Apparently activesupport v4.2.8 will drop this JSON dependency, so everything will be OK :smile:

https://github.com/flori/json/issues/303#issuecomment-269209406

Resolved in this rails PR: https://github.com/rails/rails/pull/27473

bokmann commented 7 years ago

Thanks for this. For some reason the change in the rake version has broken the jruby build. I will investigate. Also, this change will break backward compatibility with prior versions of ruby. I will likely merge the two other pull requests, do a release, then merge this and do another release, bumping the version numbers to reflect the break in backard compatibility.

tom-lord commented 7 years ago

the rake version has broken the jruby build

Yeah... I have no idea why my change could have broken that :man_shrugging:

For what it's worth, I just tried running the tests locally on my machine, and it all seems fine. Here is by bundle env:

speed 38400 baud; line = 0;
-brkint -imaxbel iutf8
git version 2.7.4
## Environment

```
Bundler   1.14.6
Rubygems  2.6.8
Ruby      2.3.1p0 (2017-01-11 revision 54768) [java]
GEM_HOME  /home/tom/.rvm/gems/jruby-9.1.7.0
GEM_PATH  /home/tom/.rvm/gems/jruby-9.1.7.0:/home/tom/.rvm/gems/jruby-9.1.7.0@global
RVM       1.29.1 (master)
Git       
Platform  universal-java-9
rubygems-bundler (1.4.4)
```

## Bundler settings

```
gem.test
  Set for the current user (/home/tom/.bundle/config): "rspec"
gem.mit
  Set for the current user (/home/tom/.bundle/config): "false"
gem.coc
  Set for the current user (/home/tom/.bundle/config): "false"
```

## Gemfile

### Gemfile

```ruby
source "https://rubygems.org"
gemspec
```

### Gemfile.lock

```
PATH
  remote: .
  specs:
    business_time (0.7.6)
      activesupport (~> 4.2.8)
      tzinfo

GEM
  remote: https://rubygems.org/
  specs:
    activesupport (4.2.8)
      i18n (~> 0.7)
      minitest (~> 5.1)
      thread_safe (~> 0.3, >= 0.3.4)
      tzinfo (~> 1.1)
    i18n (0.8.1)
    minitest (5.10.1)
    minitest-rg (5.2.0)
      minitest (~> 5.0)
    rake (12.0.0)
    rdoc (5.0.0)
    thread_safe (0.3.5)
    thread_safe (0.3.5-java)
    tzinfo (1.2.2)
      thread_safe (~> 0.1)

PLATFORMS
  java
  ruby

DEPENDENCIES
  business_time!
  minitest
  minitest-rg
  rake
  rdoc

BUNDLED WITH
   1.14.6
```

## Gemspecs

### business_time.gemspec

```ruby
$LOAD_PATH.push File.expand_path("../lib", __FILE__)
require "business_time/version"

Gem::Specification.new do |s|
  s.name = "business_time"
  s.version = BusinessTime::VERSION
  s.summary = %Q{Support for doing time math in business hours and days}
  s.description = %Q{Have you ever wanted to do things like "6.business_days.from_now" and have weekends and holidays taken into account?  Now you can.}
  s.homepage = "https://github.com/bokmann/business_time"
  s.authors = ["bokmann"]
  s.email = "dbock@javaguy.org"
  s.license = "MIT"

  s.files = `git ls-files -- {lib,rails_generators,LICENSE,README.rdoc}`.split("\n")

  s.add_dependency('activesupport','~> 4.2.8')
  s.add_dependency("tzinfo")

  s.add_development_dependency "rake"
  s.add_development_dependency "rdoc"
  s.add_development_dependency "minitest"
  s.add_development_dependency "minitest-rg"
end
```

And the test suite is all green:

Finished in 3.301594s, 45.4326 runs/s, 68.1489 assertions/s.

150 runs, 225 assertions, 0 failures, 0 errors, 0 skips
bokmann commented 7 years ago

It doesn't seem to be your issue... it looks like something about jruby and rake broke on Travis. I recreated the problem with another unrelated change, and also saw the same thing happen on another project last night. I cannot recreate locally. I can probably solve it with googling and patience.

On Mar 6, 2017, at 5:14 AM, Tom Lord notifications@github.com wrote:

the rake version has broken the jruby build

Yeah... I have no idea why my change could have broken that 🤷‍♂️

For hat it's worth, I just tried running the tests locally on my machine, and it all seems fine. Her eis by bundle env:

speed 38400 baud; line = 0; -brkint -imaxbel iutf8 git version 2.7.4

Environment

Bundler   1.14.6
Rubygems  2.6.8
Ruby      2.3.1p0 (2017-01-11 revision 54768) [java]
GEM_HOME  /home/tom/.rvm/gems/jruby-9.1.7.0
GEM_PATH  /home/tom/.rvm/gems/jruby-9.1.7.0:/home/tom/.rvm/gems/jruby-9.1.7.0@global
RVM       1.29.1 (master)
Git       
Platform  universal-java-9
rubygems-bundler (1.4.4)

Bundler settings

gem.test
  Set for the current user (/home/tom/.bundle/config): "rspec"
gem.mit
  Set for the current user (/home/tom/.bundle/config): "false"
gem.coc
  Set for the current user (/home/tom/.bundle/config): "false"

Gemfile

Gemfile

source "https://rubygems.org"
gemspec

Gemfile.lock

PATH
  remote: .
  specs:
    business_time (0.7.6)
      activesupport (~> 4.2.8)
      tzinfo

GEM
  remote: https://rubygems.org/
  specs:
    activesupport (4.2.8)
      i18n (~> 0.7)
      minitest (~> 5.1)
      thread_safe (~> 0.3, >= 0.3.4)
      tzinfo (~> 1.1)
    i18n (0.8.1)
    minitest (5.10.1)
    minitest-rg (5.2.0)
      minitest (~> 5.0)
    rake (12.0.0)
    rdoc (5.0.0)
    thread_safe (0.3.5)
    thread_safe (0.3.5-java)
    tzinfo (1.2.2)
      thread_safe (~> 0.1)

PLATFORMS
  java
  ruby

DEPENDENCIES
  business_time!
  minitest
  minitest-rg
  rake
  rdoc

BUNDLED WITH
   1.14.6

Gemspecs

business_time.gemspec

$LOAD_PATH.push File.expand_path("../lib", __FILE__)
require "business_time/version"

Gem::Specification.new do |s|
  s.name = "business_time"
  s.version = BusinessTime::VERSION
  s.summary = %Q{Support for doing time math in business hours and days}
  s.description = %Q{Have you ever wanted to do things like "6.business_days.from_now" and have weekends and holidays taken into account?  Now you can.}
  s.homepage = "https://github.com/bokmann/business_time"
  s.authors = ["bokmann"]
  s.email = "dbock@javaguy.org"
  s.license = "MIT"

  s.files = `git ls-files -- {lib,rails_generators,LICENSE,README.rdoc}`.split("\n")

  s.add_dependency('activesupport','~> 4.2.8')
  s.add_dependency("tzinfo")

  s.add_development_dependency "rake"
  s.add_development_dependency "rdoc"
  s.add_development_dependency "minitest"
  s.add_development_dependency "minitest-rg"
end

And the test suite is all green:

Finished in 3.301594s, 45.4326 runs/s, 68.1489 assertions/s.

150 runs, 225 assertions, 0 failures, 0 errors, 0 skips — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

bokmann commented 7 years ago

It seems as if the pull request #152 is a similar solution, nd is backwards compatible. do you agree?

tom-lord commented 7 years ago

No. This solution is much simpler, and is already backwards compatible.

In ruby versions < 2.4, Fixnum and Bignum both inherit from the Integer class - so this code will work just fine (as demonstrated by the green test suite, which is running on multiple versions of ruby).

I suspect that perhaps the author of #152 did not realise that this inheritance model was already present in previous ruby versions, and so thought this would be a breaking change.

bokmann commented 7 years ago

Excellent comments. Case made. Thank you.

On Mar 25, 2017, at 3:29 PM, Tom Lord notifications@github.com wrote:

No. This solution is much simpler, and is already backwards compatible.

In ruby versions < 2.4, Fixnum and Bignum both inherit from the Integer class - so this code will work just fine (as demonstrated by the green test suite, which is running on multiple versions of ruby).

I suspect that perhaps the author of #152 did not realise that this inheritance model was already present in previous ruby versions, and so thought this would be a breaking change.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

carlzulauf commented 7 years ago

This is correct. I made assumptions about Integer not always being present in the ancestors tree, but it is always there and has been since at least ruby 1.8. I saw other patches using 0.class in other gems but that wouldn't be needed here. Always placing these extensions in Integer makes sense.

bokmann commented 7 years ago

Thanks for this! In the live presentation I mentioned earlier, and merging!