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

chat: de-linting #18823

Merged
merged 3 commits into from Aug 14, 2019

Conversation

@strib
Copy link
Contributor

commented Aug 7, 2019

Issue: HOTPOT-492

@strib strib requested review from mlsteele, joshblum and mmaxim Aug 7, 2019

@strib strib force-pushed the strib/HOTPOT-492-delint-chat branch from c27e72e to 71a46d0 Aug 7, 2019

@@ -200,43 +200,3 @@ func (s *Source) PreviewBuiltinCommand(ctx context.Context, uid gregor1.UID, con
cmd.Preview(ctx, uid, convID, tlfName, text)
}
}

This comment has been minimized.

Copy link
@mmaxim

mmaxim Aug 8, 2019

Member

I'd like to keep this here

This comment has been minimized.

Copy link
@strib

strib Aug 8, 2019

Author Contributor

Ok done

strib added a commit that referenced this pull request Aug 8, 2019

chat: restore admin code
Suggested by mikem.

Issue: #18823

@strib strib requested a review from mmaxim Aug 8, 2019

@strib strib force-pushed the strib/HOTPOT-492-delint-chat branch from 429c9a7 to cb53430 Aug 8, 2019

strib added a commit that referenced this pull request Aug 8, 2019

chat: restore admin code
Suggested by mikem.

Issue: #18823
@mmaxim

mmaxim approved these changes Aug 9, 2019

Copy link
Member

left a comment

Couple things:

  1. Thanks for the thoughtful changes! I was surprised that a lot of these instances of missing error handling seemed to have all been considered.
  2. I'm not a huge fan of the turn else { if { into else if rule. Sometimes I like to just logically eparate the two branches, and if the else branch happens to be doing an error check it could just stay in there.
  3. I'm also not a huge fan of the rule where functions called with go out front require the error to be checked. I think if you are putting the function call deliberately into the background, then you are making a strong signal that the error return doesn't matter.
@strib

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

Thanks for looking! On your last two points, does "not a huge fan" mean you want to make a case for disabling those rules across all of the Keybase projects? I don't know that there's much we can do about the 3rd point though, as it's just part of the general error checking rule. Basically, we would either need to change the function being go'd to not return an error, or wrap it in something that discards the error.

@mmaxim

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

I'd be up for taking out #2 for sure.

@strib

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

Ok. Maybe on Monday we can take another poll. I don't want to make the decision based just on someone who has a strong opinion. I personally think it makes the code easier to read to not nest the if unnecessarily, so I don't think it's an obvious call.

@strib strib force-pushed the strib/HOTPOT-492-delint-chat branch from cb53430 to fdbf848 Aug 14, 2019

strib added a commit that referenced this pull request Aug 14, 2019

chat: restore admin code
Suggested by mikem.

Issue: #18823

strib added a commit that referenced this pull request Aug 14, 2019

chat: back out `elseif`-related changes
Suggested by mikem.

Issue: #18823

strib added some commits Aug 5, 2019

chat: de-linting
Issue: HOTPOT-492
chat: restore admin code
Suggested by mikem.

Issue: #18823
chat: back out `elseif`-related changes
Suggested by mikem.

Issue: #18823

@strib strib force-pushed the strib/HOTPOT-492-delint-chat branch from fdbf848 to 8588bc1 Aug 14, 2019

@strib strib merged commit 5a9efb8 into master Aug 14, 2019

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@strib strib deleted the strib/HOTPOT-492-delint-chat branch Aug 14, 2019

strib added a commit that referenced this pull request Aug 14, 2019

chat: restore admin code
Suggested by mikem.

Issue: #18823
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.