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

Remove Kwalify #1757

Closed
bkuhlmann opened this issue Dec 29, 2023 · 4 comments
Closed

Remove Kwalify #1757

bkuhlmann opened this issue Dec 29, 2023 · 4 comments

Comments

@bkuhlmann
Copy link

Why

Hello and Happy Holidays. 👋

I'd like to suggest removing the Kwalify gem dependency from Reek. This dependency hasn't been maintained in years and when visiting the gem, there doesn't appear to be a valid link to the source code anymore. In addition, in Ruby 3.3.0, Kwalify is throwing performance warnings due to Object Shape violations. This means, every time I use Reek, I see these warnings show up.

How

To reproduce, run the following:

RUBYOPT="-W:performance" reek

Notice you get the following warning:

/Users/demo/.cache/frum/versions/3.3.0/lib/ruby/gems/3.3.0/gems/kwalify-0.7.2/lib/kwalify/rule.rb:391: warning: The class Kwalify::Rule reached 8 shape variations, instance variables accesses will be slower and memory usage increased.
It is recommended to define instance variables in a consistent order, for instance by eagerly defining them all in the #initialize method.

Notes

  • OS: macOS 13.6.3 (22G436)
  • Ruby: ruby 3.3.0 (2023-12-25 revision 5124f9ac75) +YJIT [arm64-darwin22.6.0]
  • Reek: 6.1.4
@mvz
Copy link
Collaborator

mvz commented Dec 29, 2023

In fact, kwalify was already replaced with dry-schema in #1749.

I guess it's about time to release a new version.

@bkuhlmann
Copy link
Author

bkuhlmann commented Dec 29, 2023

Ah, good. Didn't realize work for that is complete. Yeah, a new version would be great! 🙇

I'll close this issue based on the above info.

@mvz
Copy link
Collaborator

mvz commented Dec 31, 2023

@bkuhlmann Reek 6.2.0 has now been released including the change from kwalify to dry-schema.

@bkuhlmann
Copy link
Author

Thanks! I've updated all of my projects accordingly. Nice to see this warning resolved now. 😉

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

No branches or pull requests

2 participants