Add official testing for Ruby 3.0 #101

JacobEvelyn · 2021-01-05T19:34:19Z

Before merging:

Copy the latest benchmark results into the README.md and update this PR
If this change merits an update to CHANGELOG.md, add an entry following Keep a Changelog guidelines with semantic versioning

codecov · 2021-01-05T19:34:54Z

Codecov Report

Merging #101 (d5b4671) into main (4bdb862) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main      #101   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          621       621           
=========================================
  Hits           621       621

Impacted Files	Coverage Δ
lib/memo_wise.rb	`100.00% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4bdb862...c8c5558. Read the comment docs.

JacobEvelyn · 2021-02-01T21:55:38Z

README.md

@@ -78,20 +78,23 @@ For more usage details, see our detailed [documentation](#documentation).

 ## Benchmarks


I'd recommend viewing either the rich diff or the end result rendered as markdown.

JacobEvelyn · 2021-02-01T21:56:09Z

benchmarks/benchmarks.rb

@@ -115,7 +121,7 @@ def positional_splat_keyword_and_double_splat_args(a, *args, b:, **kwargs)
    # retrieval time.
    instance.no_args

-    x.report("#{benchmark_gem.benchmark_name}: no_args") { instance.no_args }
+    x.report("#{benchmark_gem.benchmark_name}: ()") { instance.no_args }


I updated the names of benchmarks to make it clearer where they go in the README.

JacobEvelyn · 2021-02-01T21:56:28Z

benchmarks/benchmarks.rb

    ) do
-      ARGUMENTS.each { |a, b| instance.positional_and_splat_args(a: a, b: b) }


Looks like this was a bug! (It's calling the wrong method 😓)

JacobEvelyn · 2021-02-01T21:56:39Z

benchmarks/benchmarks.rb

    ) do
      ARGUMENTS.each do |a, b|
-        instance.positional_and_splat_args(a, b, a: a, b: b)


Another bug! (It's calling the wrong method 😓)

JacobEvelyn · 2021-02-01T21:59:00Z

benchmarks/benchmarks.rb

@@ -216,15 +224,15 @@ def positional_splat_keyword_and_double_splat_args(a, *args, b:, **kwargs)
    # Run once with each set of arguments to memoize the result values, so our
    # benchmark only tests memoized retrieval time.
    ARGUMENTS.each do |a, b|
-      instance.positional_splat_keyword_and_double_splat_args(a, b, a: a, b: b)
+      instance.positional_splat_keyword_and_double_splat_args(a, b, b: b, a: a)


I switched the order of these args because b: b is the keyword arg and a: a is **kwargs and I'm worried that a: a, b: b might put them both in through **kwargs and bypass the keyword arg.

jemmaissroff · 2021-02-01T22:24:56Z

CHANGELOG.md

@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Logo
 - Memoization of class methods
 - Support for instances created with `Class#allocate`
+- Official testing and benchmarks for Ruby 3.0


Worth noting that the code itself was also upgraded to have a ruby version of 3.0?

Do you mean the .ruby-version file? I'm not sure that's useful, since that's only used for local development and not users of the gem or our automated tests. Or am I misunderstanding?

No you're totally right, good point, it's internal to the gem.

jemmaissroff · 2021-02-01T22:29:07Z

benchmarks/benchmarks.rb

+
+# Some gems do not yet work in Ruby 3 so we only require them if they're loaded
+# in the Gemfile.
+%w[memery memoist].each { |gem| require gem if Gem.loaded_specs.key?(gem) }


No strong preference here either, but just looking again, could we include memoized and memoizer in the first line to make the second line also a one liner?

%w[memery memoist memoized memoizer].each { |gem| require gem if Gem.loaded_specs.key?(gem) } # The VERSION constant does not get loaded above. %w[memoized memoizer].each { |gem| require "#{gem}/version" if Gem.loaded_specs.key?(gem) }

Nice suggestion! Unfortunately those lines are too long. The best we could do would be:

# Some gems do not yet work in Ruby 3 so we only require them if they're loaded # in the Gemfile. %w[memery memoist memoized memoizer]. each { |gem| require gem if Gem.loaded_specs.key?(gem) } # The VERSION constant does not get loaded above. %w[memoized memoizer]. each { |gem| require "#{gem}/version" if Gem.loaded_specs.key?(gem) }

Up to you; I'm happy with whichever you prefer.

I think I like this better than what we have, if you don't mind changing it! Reads cleaner to me

jemmaissroff · 2021-02-02T13:08:51Z

CHANGELOG.md

@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Logo
 - Memoization of class methods
 - Support for instances created with `Class#allocate`
+- Official testing and benchmarks for Ruby 3.0


No you're totally right, good point, it's internal to the gem.

jemmaissroff · 2021-02-02T13:09:20Z

benchmarks/benchmarks.rb

+
+# Some gems do not yet work in Ruby 3 so we only require them if they're loaded
+# in the Gemfile.
+%w[memery memoist].each { |gem| require gem if Gem.loaded_specs.key?(gem) }


I think I like this better than what we have, if you don't mind changing it! Reads cleaner to me

ms-ati

Looks great!

JacobEvelyn force-pushed the ruby-3 branch from 3a4e050 to 96f9866 Compare January 5, 2021 20:01

JacobEvelyn force-pushed the ruby-3 branch 8 times, most recently from f8ab2a9 to 684c795 Compare February 1, 2021 21:54

JacobEvelyn marked this pull request as ready for review February 1, 2021 21:54

JacobEvelyn requested review from jemmaissroff and ms-ati February 1, 2021 21:54

JacobEvelyn commented Feb 1, 2021

View reviewed changes

JacobEvelyn force-pushed the ruby-3 branch from 684c795 to 7165bad Compare February 1, 2021 21:57

JacobEvelyn commented Feb 1, 2021

View reviewed changes

JacobEvelyn force-pushed the ruby-3 branch from 7165bad to befcf68 Compare February 1, 2021 22:05

jemmaissroff suggested changes Feb 1, 2021

View reviewed changes

jemmaissroff suggested changes Feb 2, 2021

View reviewed changes

ms-ati approved these changes Feb 2, 2021

View reviewed changes

jemmaissroff approved these changes Feb 2, 2021

View reviewed changes

Add official testing for Ruby 3.0

c8c5558

JacobEvelyn force-pushed the ruby-3 branch from 5981cec to c8c5558 Compare February 2, 2021 15:57

JacobEvelyn merged commit 7e7786a into main Feb 2, 2021

JacobEvelyn deleted the ruby-3 branch February 2, 2021 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add official testing for Ruby 3.0 #101

Add official testing for Ruby 3.0 #101

JacobEvelyn commented Jan 5, 2021 •

edited

codecov bot commented Jan 5, 2021 •

edited

JacobEvelyn Feb 1, 2021

JacobEvelyn Feb 1, 2021

JacobEvelyn Feb 1, 2021 •

edited

JacobEvelyn Feb 1, 2021 •

edited

JacobEvelyn Feb 1, 2021

jemmaissroff Feb 1, 2021

JacobEvelyn Feb 1, 2021

jemmaissroff Feb 2, 2021

jemmaissroff Feb 1, 2021

JacobEvelyn Feb 1, 2021

jemmaissroff Feb 2, 2021

jemmaissroff Feb 2, 2021

jemmaissroff Feb 2, 2021

ms-ati left a comment

		@@ -78,20 +78,23 @@ For more usage details, see our detailed [documentation](#documentation).

		## Benchmarks

		) do
		ARGUMENTS.each { \|a, b\| instance.positional_and_splat_args(a: a, b: b) }

Add official testing for Ruby 3.0 #101

Add official testing for Ruby 3.0 #101

Conversation

JacobEvelyn commented Jan 5, 2021 • edited

codecov bot commented Jan 5, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobEvelyn Feb 1, 2021 • edited

Choose a reason for hiding this comment

JacobEvelyn Feb 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-ati left a comment

Choose a reason for hiding this comment

JacobEvelyn commented Jan 5, 2021 •

edited

codecov bot commented Jan 5, 2021 •

edited

JacobEvelyn Feb 1, 2021 •

edited

JacobEvelyn Feb 1, 2021 •

edited