Conversation
|
I'm not sure all of these need the extra return as it looks like they're falling through to an error return outside of the type check. Basically the pattern looks subtly different to the ones we saw for the rulesets. I'm on my phone so will look again in the morning on a big screen to double check. |
| d.SetId("") | ||
| return nil | ||
| } | ||
| return err |
There was a problem hiding this comment.
This one looks redundant, since we already have return err below.
| } | ||
| return err | ||
| } | ||
| return err |
There was a problem hiding this comment.
This is the only one that doesn't return a redundant error on this PR, I think. But I would still remove the return err above.
There was a problem hiding this comment.
From what I'm checking in a quick glance, it seems most of these returns are redundants, as there's no code path after the if and most of them falls to the return err below. Can you double-check if that's true?
EDIT: I just saw @stevehipwell's comments, and I mostly agree with him.
|
@stevehipwell @diofeher let me do another sweep to make sure I didn't over index on my search of missing instances. Thx for the quick 👀 |
…ons/terraform-provider-github into 2957-fix-missing-error-returns
stevehipwell
left a comment
There was a problem hiding this comment.
I think you've got the obviously broken one and it looks like the other instances have already been cleaned up. I did spot the following related patterns that aren't idiomatic (using else where it's not required) and could do with being cleaned up.
| if err != nil { | ||
| return err | ||
| } | ||
| } else { |
There was a problem hiding this comment.
question: it looks like the else might be redundant here or are there legitimate use-case where it's fine to swallow err?
There was a problem hiding this comment.
I think there is. When create fails but update succeeds.
Resolves #2957
Relates to: #2705
Before the change?
After the change?
Added catch all for:
Pull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!