search

troessner / reek

Code smell detector for Ruby
https://github.com/troessner/reek
MIT License
4k stars 279 forks source link

Add `Tempfile.create` to ignored iterators list #1747

Closed mateusdeap closed 7 months ago

mateusdeap commented 8 months ago

Fixes #1490

I've added Tempfile.create to the list of ignored iterators and added a spec for it.

I'm submitting the PR to also get some feedback on the implementation. The specs all pass but it fails on the manual method dispatch code smell.

I tried searching for alternative ways to implement this without using respond_to? but didn't really find anything satisfactory, and if I don't test for that beforehand, many specs fail on exceptions because the exp object lacks the receiver method. Feedback on how to fix this would be most welcome!

Otherwise, I hope this is useful!

troessner commented 7 months ago

Looks good to me! @mvz ?

mvz commented 7 months ago

I'll have a look this evening.

mvz commented 7 months ago

The code looks good, but there are two issues:

  1. For some reason, ManualDispatch complains that "Reek::SmellDetectors::NestedIterators#ignored_iterator? manually dispatches method call". I don't see that, so maybe that's a bug in ManualDispatch?
  2. @troessner if I read https://github.com/troessner/reek/issues/1490#issuecomment-525278433 correctly, you wanted to allow the option of ignoring Tempfile.create but not have that method be added to the default list of ignored methods?
mvz commented 7 months ago

For some reason, ManualDispatch complains that "Reek::SmellDetectors::NestedIterators#ignored_iterator? manually dispatches method call". I don't see that, so maybe that's a bug in ManualDispatch?

Ah, that detector simply finds calls to #respond_to?, so I guess that makes sense.

mateusdeap commented 7 months ago

@troessner if I read https://github.com/troessner/reek/issues/1490#issuecomment-525278433 correctly, you wanted to allow the option of ignoring Tempfile.create but not have that method be added to the default list of ignored methods?

@mvz As for this, now that I'm rereading it, I'm not sure. I interpreted it at the time as being "A PR that whitelists Tempfile.create". But if it's an option, than I suppose it would be something like a whitelist section in the reek configuration?

mvz commented 7 months ago

But if it's an option, than I suppose it would be something like a whitelist section in the reek configuration?

Yes, it would be the ignore_iterators section just like here: https://github.com/troessner/reek/pull/1747/files#diff-473a12b36b25df9c36979c53cb67f2dce02afc614dca0c971b96154a7474d483R60, except in the user's own .reek.yml file.

mateusdeap commented 7 months ago

I see. In that case, I'm not sure, honestly.

Now that I think about it, maybe I just leave the addition of the Tempfile.create item in the default reek.yml and that would be good enough?

Because I still get the impression the idea was that Tempfile.create (and some other methods, I now notice) are getting flagged with this smell even though they actually aren't cases of nested iterators. Basically all situations where the method yields a file that is open and closed within the loop.

Now I'm not so sure about the PR anymore lol

mvz commented 7 months ago

Now I'm not so sure about the PR anymore lol

I think the change to #ignored_iterator? is still very useful.

@troessner can you enlighten us about your comment https://github.com/troessner/reek/issues/1490#issuecomment-525278433?

troessner commented 7 months ago

Puhh, that's 4 years ago and I haven't really any context anymore regarding this. Feel free to ignore my comment ;)