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

Support for newer rubies #63

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Support for newer rubies #63

wants to merge 3 commits into from

Conversation

pjaspers
Copy link
Contributor

@pjaspers pjaspers commented Apr 30, 2020

This adds support for rubies greater than 2.5 (including the newish ruby 2.7)

In 2.6 they deprecated BigDecimal.new which was used by Dentaku so I had to upgrade, Dentaku as well, they added some functions we already added (but we don't seem to clash).

The other big changes are ruby being more strict about calling functions with keyword arguments.

I've taken some timings along the way (by running the test suite three times):

Baseline (without) any changes: 10.66s

- Finished in 10.55 seconds (files took 1.53 seconds to load) - Finished in 11.77 seconds (files took 1.03 seconds to load) - Finished in 9.67 seconds (files took 1.14 seconds to load)

After bundle update: 10.85s

Finished in 11.05 seconds (files took 1.74 seconds to load) Finished in 10.16 seconds (files took 1.01 seconds to load) Finished in 11.35 seconds (files took 1.14 seconds to load)

Dentaku update: 10.6s

Finished in 10.9 seconds (files took 1.48 seconds to load) Finished in 10.47 seconds (files took 1.03 seconds to load) Finished in 10.43 seconds (files took 1.08 seconds to load)

Ruby 2.7.1: 10.86s

(these are chronologically, so the ruby 2.7.1 has all the other updates in it as well)

So I don't really see any big regressions, but the proof in the pudding will be to run the tests in orbf2 with newer rubies. Based on this, we know that the engine will supports them.

Travis will now run the test suite for all three ruby versions: 2.5, 2.6 and 2.7

pjaspers added 3 commits April 30, 2020 15:44
This is mostly to do with the keyword arguments being more
strict. Where we used to be able to just throw a hash to a method, we
now need to explicitly tell ruby to treat the hash as keyword
arguments. This fixes these calls.

We have a couple of places in our used gems where this still pops up:

      /Users/pjaspers/development/blsq/orbf-rules_engine/vendor/ruby/2.7.0/gems/dentaku-3.3.4/lib/dentaku/exceptions.rb:95: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
      /Users/pjaspers/development/blsq/orbf-rules_engine/vendor/ruby/2.7.0/gems/dentaku-3.3.4/lib/dentaku/exceptions.rb:78:
      warning: The called method `initialize' is defined here

And

      /Users/pjaspers/development/blsq/orbf-rules_engine/vendor/ruby/2.7.0/gems/dhis2-2.3.8/lib/dhis2/collection_wrapper.rb:12: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
      /Users/pjaspers/development/blsq/orbf-rules_engine/vendor/ruby/2.7.0/gems/dhis2-2.3.8/lib/dhis2/api/analytic.rb:7: warning: The called method `list' is defined here
Explicitly install newer bundler (travis defaults to 1.x bundler), on
ruby 2.7 this step isn't needed, but the other ones do need it.

Since the bundler will be cached after the first install let's keep it
like this.
@pjaspers pjaspers changed the title Pj/support newer rubies Support for newer rubies Apr 30, 2020
@pjaspers pjaspers marked this pull request as ready for review April 30, 2020 15:05
@pjaspers pjaspers requested a review from mestachs April 30, 2020 15:06
@mestachs
Copy link
Contributor

my timings

  • master : Finished in 6.74 seconds (files took 0.69376 seconds to load)
  • 2.5 & branch : Finished in 6.96 seconds (files took 0.70392 seconds to load)
  • 2.6.6 & branch : Finished in 7.82 seconds (files took 0.72265 seconds to load)
  • 2.7.1 & branch : Finished in 7.78 seconds (files took 0.73129 seconds to load)

2.7.1 causing a lot of warnings :

.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/mime-types-3.3/lib/mime/types/logger.rb:30: warning: `_1' is reserved for numbered parameter; consider another name
.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/mime-types-3.3/lib/mime/types/logger.rb:30: warning: `_2' is reserved for numbered parameter; consider another name
.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/mime-types-3.3/lib/mime/types/logger.rb:30: warning: `_3' is reserved for numbered parameter; consider another name
.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/json-2.2.0/lib/json/common.rb:156: warning: Using the last argument as keyword parameters is deprecated

did a bundle update and re-run on 2.7.1, most warnings disappeared
Finished in 7.54 seconds (files took 0.72817 seconds to load)

That's still a lot slower for compared to 2.5.
I remember dropping the ** or * usage significantly improved the perf.

@mestachs
Copy link
Contributor

2.7.1 reverting https://github.com/BLSQ/orbf-rules_engine/pull/63/files#diff-2d8c0e5a8f1f0a37f717eae85275cba7R73

Finished in 7.23 seconds (files took 0.70755 seconds to load)

@mestachs
Copy link
Contributor

pkill firefox
pkill code
with the revert
ruby 2.7.1 Finished in 7.07 seconds (files took 0.68045 seconds to load)
ruby 2.5.1 Finished in 6.61 seconds (files took 0.68011 seconds to load)

@pjaspers
Copy link
Contributor Author

OK, I'll look into the revert. See if I can avoid the **

@mestachs
Copy link
Contributor

now going to bed... but why don't you observe the same timings...

@pjaspers
Copy link
Contributor Author

I have a much slower computer :) But the results are kind of in line (and the double splat does make a big difference).

My fix for the warning was actually not really the right solution to the problem.

class Hi
  def initialize(hash)
    puts hash
  end

  def self.with(hash)
    new(hash)
  end
end

class HiAgain < Hi
  def initialize(it: me)
    # Because we're overriding the initialize with keywords
    # ruby is throwing the warning
  end
end

hi = Hi.with(a: "b", c: "d")
HiAgain.with(it: "me") # this will throw the waring

Is my reduced case, where I can trigger the warning.

@pjaspers pjaspers mentioned this pull request Sep 14, 2020
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.

2 participants