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

Fix kwargs patching #313

Merged
merged 2 commits into from Apr 3, 2024
Merged

Conversation

simpl1g
Copy link
Contributor

@simpl1g simpl1g commented Mar 28, 2024

Fixes #312

@simpl1g
Copy link
Contributor Author

simpl1g commented Mar 28, 2024

Also it looks like in Ruby 3.2/3.3 prepend works with the same speed as alias
I did some comparison here https://gist.github.com/simpl1g/e34c073a8e33128d1f309873d9164967 and even with empty work there is no difference in speed. Maybe it is time to make prepend as default instrumentation, because it gives more benefits.

Comparison:
              method:  2189776.7 i/s
   method_triple_dot:  2178433.1 i/s - same-ish: difference falls within error
  prepend_triple_dot:  2161501.4 i/s - same-ish: difference falls within error
      method_prepend:  2135422.1 i/s - same-ish: difference falls within error

@SamSaffron
Copy link
Member

I think I am fine with this ... crazy that 2.7 was EOL a year ago!

CI is failing though, can you have a look?

@simpl1g
Copy link
Contributor Author

simpl1g commented Apr 2, 2024

Thank you for review!

Looks like CI is failing for a long time, it is connected with rubocop-discourse gem, it is installing different version for different gemfiles. And in new version there are some changes in cops which break CI. So I made changes to rubocop config

BUNDLE_GEMFILE=gemfiles/ar_60.gemfile bundle list | grep rubocop-discourse
* rubocop-discourse (3.6.1)

BUNDLE_GEMFILE=gemfiles/ar_61.gemfile bundle list | grep rubocop-discourse
* rubocop-discourse (3.7.1)

Also ar_70.gemfile actually didn't test 7.0, but 6.0. Fixed this as well

BUNDLE_GEMFILE=gemfiles/ar_70.gemfile bundle list | grep active
  * activemodel (6.0.6.1)
  * activerecord (6.0.6.1)
  * activesupport (6.0.6.1)

@@ -0,0 +1 @@
--ignore-unrecognized-cops
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is required for compatibility with older versions of rubocop-discourse

@SamSaffron
Copy link
Member

This is certainly a radical change (deliberately breaking releases prior to Ruby 3.0) , but I am going to accept it @tgxworld / @davidtaylorhq it is more correct and probably something we should pull into method profiler on Discourse.

I guess at the end of the day people that need Ruby 2 support can just install earlier gems.

Thanks @simpl1g

@SamSaffron SamSaffron merged commit e23bac5 into discourse:main Apr 3, 2024
14 checks passed
@simpl1g simpl1g deleted the fix-kwargs-patching branch April 3, 2024 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect keyword arguments handling in MethodProfiler
2 participants