Skip to content

Conversation

lukebooth
Copy link

This does remove the appraisals for <6. I don’t know that we have to make that call, but if we’re going to make a new 2.0.x version for these changes, I think it’d be ok to say you need this version if you’re on Rails 6+

Copy link
Contributor

@kobsy kobsy left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks for digging into this, @lukebooth

Copy link
Owner

@boblail boblail left a comment

Choose a reason for hiding this comment

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

👋🏼 Hey guys!!

@@ -35,8 +35,9 @@ def pluck

def benchmark(title)
result = nil
ms = Benchmark.ms { result = yield }
PluckMap.logger.info "\e[33m#{title}: \e[1m%.1fms\e[0m" % ms
benchmark_klass = ActiveRecord.version.to_s.split(".").first.to_i > 7 ? ActiveSupport::Benchmark : Benchmark
Copy link
Owner

Choose a reason for hiding this comment

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

Isn't Benchmark in the standard library? What prevents from just using ::Benchmark all the time?

Copy link
Author

Choose a reason for hiding this comment

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

You're not wrong 😅
I tried that first and was still getting an error running the appraisal for Rails 8. I guess Rails doesn't require it for us anymore at that point. Adding a require "benchmark" fixed it. But I did keep the realtime change because ms will be removed in 8.1.

This does remove the appraisals for <6. I don’t know that we _have_ to make that call, but if we’re going to make a new 2.0.x version for these changes, I think it’d be ok to say you need this version if you’re on Rails 6+
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.

3 participants