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!!

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