Add Tempfile.create to ignored iterators list

[mateusdeap]

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]

Looks good to me! @mvz ?

[mvz]

I'll have a look this evening.

[mvz]

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 NestedIterator should ignore _currently open_ like methods (like CSV and tempfile). #1490 (comment) 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]

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.

[mvz]

The following seems to work and won't trigger ManualDispatch:

/#{pattern}/ =~ call.format_to_ruby

[mateusdeap]

Interesting. I never heard of that method. Will edit the PR

[mvz]

It's a method defined on AST nodes by Reek itself.

[mateusdeap]

Applied changes

[mateusdeap]

@troessner if I read #1490 (comment) 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]

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]

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]

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 #1490 (comment)?

[troessner]

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

[mvz]

Alright then, let's merge this!