Add Path#resolve? macro method #4408
Conversation
|
Just added a much cleaner implementation which doesn't rely on catching the thrown exception and inspecting it's message. |
|
Would you add one or two lines of documentation? Documentation for macros is in Btw, thanks for adding a couple of tests for |
|
Oh! I forgot about the macro docs, silly me. I'll make sure to get that done. |
| unless matched_type | ||
| node.raise_undefined_constant(@path_lookup) | ||
| end | ||
| return unless matched_type |
There was a problem hiding this comment.
Wouldn't it be more explicit to return nil unless matched_type here?
There was a problem hiding this comment.
Leaving out nil seems to be the more prevalent style so far, but there's quite a bit with nil too, and I don't mind either way. If we want to be consistent with return nil (if|unless), then maybe we should disallow not specifying it in the compiler (or formatter).
$ grep -ER 'return (unless|if)' src | wc -l
264
$ grep -ER 'return nil (unless|if)' src | wc -l
81
There was a problem hiding this comment.
That's an interesting stat! I tend to prefer more explicitness given early returning "generates" an union. Then when you get the famous "Nil does not respond to etc." it is easier to see how it can be Nil. But I guess my opinion on that is not as pervasive as I would have thought :(
There was a problem hiding this comment.
Explicitness is noisy. Here nil is implied, just like return is implied when returning a value.
There was a problem hiding this comment.
I don't agree in this case but one has to pick his fights :)
|
I added some documentation, it should be ready to merge now. |
|
Thanks @RX14! 🙇 |
|
@mverzilli I think you can safely remove |
I think this is a useful feature, as most
ASTNodesallow you to assert in some way whether the call you're about to make will raise, howeverPath#resolvedoes not.Closes #4370.