Add binding support for keyword arguments that override native keywords

[bkuhlmann]

Overview

Hello. :wave: I realize this is a rare use case but, occasionally, I like to override native keywords for more expressive messaging.

Steps to Recreate

def call content, for: :asciidoc
  renderers.fetch(binding.local_variable_get(:for)).call content
end

Reek will flag the above as UnusedParameters which, normally, is true except I can't use for explicitly because it'll cause a syntax error due to conflicts with the native keyword so pulling from the binding is a nice workaround. It would be nice if Reek could recognize this and bypass the error in this situation.

Environment

• Ruby: ruby 3.2.2 (2023-03-30 revision e51014f9c0) +YJIT [arm64-darwin22.4.0]
• Reek: 6.1.4

[mvz]

I think in this case I would make the message go away using a comment:

# :reek:UnusedParameters
def call(content, for: :asciidoc)
  renderers.fetch(binding.local_variable_get(:for)).call content
end

[bkuhlmann]

Yep and I do that too as a temporary workaround before my final refactoring of code where I have to delete the Reek comment and move it into my .reek.yml configuration. Otherwise, my Git Hooks will flag the code comment as part of my refactoring work before I publish a new version of code.

Reek is a great coding buddy so always want to make sure those code comments are addressed.

In this case, I see this pattern pop up enough that I'd like to reduce the steps needed for code comments and/or updating the global .reek.yml entirely. 😅

[mvz]

You seem to have very strict checks 🤔. If you look at Reek's source code you will see that it even has code comments to disable smell detection in some places 😆

I must say using binding in this way makes the code itself look more complex, so I'm not eager to change Reek to encourage this pattern.

So .. maybe a workable solution would be to facilitate adding configuration in .reek.yml that would ignore just for as an unused argument. It seems this currently is not working using the exclude configuration option. That looks like a bug to me.

[bkuhlmann]

You seem to have very strict checks.

True. As a bootstraper, fellow open source maintainer, and one struggling to get my SaaS off the ground I can't afford to make mistakes. I also need a team of engineers that never tire and always keep me honest. Reek is one of the best in this regard. I've written about this in the past in case it helps illuminate more as to why I think Reek and fellow tools like this are so valuable. I often call this augmented engineering which, in today's age of AI, I think is finally starting to seep in the zeitgeist more.

If you look at Reek's source code you will see that it even has code comments to disable smell detection in some places

Yeah, I have and kind of makes me sad (like the project's maintainability score is Rated C). 😅 ...but as the maintainer of Git Lint I think it's super important to practice what you preach. If you don't adhere to your own tools then it's hard to be champion and beacon of light for others to follow and be inspired by.

I must say using binding in this way makes the code itself look more complex, so I'm not eager to change Reek to encourage this pattern.

I agree and should be used with caution. ...but every now and then, the syntactic sugar you get from overriding Ruby's native keyword can make a difference in they way the messages between your objects look and feel. It's the small touches in life that sometimes make the difference.

[M]aybe a workable solution would be to facilitate adding configuration in .reek.yml that would ignore just for as an unused argument.

Actually, I am able to make this work:

UnusedParameters:
  exclude:
    - Milestoner::Renderers::Universal#call

...but if I was able to exclude any method that used :for -- or similar -- as a keyword argument that'd be neat because then you'd not need to point to specific objects/methods and have more control over general keywords.

If it's too much trouble and don't feel this is worth pursuing then I understand but I do like your point about having a bit more configurability at a more generic level. :bow:

[mvz]

I think it would not be worth detecting use of the for parameter via local_variable_get. Instead, I would suggest the solution is to pick any other parameter name. In this case for example, something like for_renderer may be clearer.

In conclusion, I'm going to close this as 'won't fix'.