Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: change the isError to not use the state #7483

Merged
merged 2 commits into from
May 13, 2021
Merged

fix: change the isError to not use the state #7483

merged 2 commits into from
May 13, 2021

Conversation

wcarlson5
Copy link
Contributor

@wcarlson5 wcarlson5 commented May 6, 2021

since the state ERROR changed definitions the current isError is no longer really relevant. However it actually seems that isError is not used so I will just remove it._

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@wcarlson5 wcarlson5 requested a review from a team as a code owner May 6, 2021 21:04
@ghost
Copy link

ghost commented May 6, 2021

@confluentinc It looks like @wcarlson5 just signed our Contributor License Agreement. 👍

Always at your service,

clabot

Copy link
Contributor

@ableegoldman ableegoldman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me

Copy link
Contributor

@guozhangwang guozhangwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @wcarlson5 Could you update the description a bit for the rationales just for my education? Why we should not use the state for isError?

@wcarlson5
Copy link
Contributor Author

@guozhangwang since the state ERROR changed definitions the current isError is no longer really relevant. However it actually seems that isError is not used so I will just remove it

@ableegoldman
Copy link
Contributor

+1 on putting what you just said in the PR description. Filling out the description just makes it easier to understand what the PR is about at a glance, which is useful eg if we ever have to skim over PRs looking for possible regressions. It's a good idea to always do, even if the changes in the PR are extremely obvious to you

@wcarlson5
Copy link
Contributor Author

good call. TBH I was going to check with some more testing to see if I needed to remove it (which I can). I probably should have made it as a draft PR first. But thanks for the quick reviews!

@wcarlson5 wcarlson5 merged commit 41ec430 into master May 13, 2021
@wcarlson5 wcarlson5 deleted the fix_isError branch May 13, 2021 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants