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

improve code quality #1064

Closed
wants to merge 7 commits into from
Closed

improve code quality #1064

wants to merge 7 commits into from

Conversation

withshubh
Copy link
Contributor

@withshubh withshubh commented Feb 25, 2021

Description

This PR fixes a few issues that were affecting the code quality.

Summary of changes

  • Fix unnecessary calls to Printf
  • Fix nil context being passed to function
  • Remove empty default in select
  • Replace strings.Index with strings.Contains
  • Replace bytes.Compare with bytes.Equal
  • Merge variable declaration with assignment
  • Remove unnecessary use of `fmt.Sprintf

@withshubh withshubh mentioned this pull request Feb 25, 2021
2 tasks
default:

Copy link
Member

@peterbourgon peterbourgon May 25, 2021

Choose a reason for hiding this comment

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

This results in different behavior, does it not?

Copy link
Contributor

@sagikazarmark sagikazarmark Jun 19, 2021

Choose a reason for hiding this comment

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

It doesn't change the observed behavior (it'll still block until either one of the first two cases return something).

TBH I don't really understand why the for loop is necessary.

Copy link
Member

@peterbourgon peterbourgon Jun 19, 2021

Choose a reason for hiding this comment

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

Well if you have the default, you need the for, otherwise this is a one-and-done.

But it's still a terrible pattern, as it turns what could be a simple select into a hot loop?? Maybe an artifact from refactoring.

Copy link
Contributor

@sagikazarmark sagikazarmark Jun 19, 2021

Choose a reason for hiding this comment

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

Probably, yeah. @withshubh could you please remove the for loop as well?

Copy link
Member

@peterbourgon peterbourgon left a comment

Waiting for the fix to sd/etcd

@peterbourgon peterbourgon requested a review from sagikazarmark Jul 18, 2021
@sagikazarmark sagikazarmark mentioned this pull request Jul 20, 2021
@sagikazarmark
Copy link
Contributor

sagikazarmark commented Jul 20, 2021

Apparently the user deleted the fork. I resubmitted the PR in #1168

@peterbourgon
Copy link
Member

peterbourgon commented Jul 20, 2021

Unless you intend to fix the etcd issue too, I'd just close it.

@sagikazarmark
Copy link
Contributor

sagikazarmark commented Jul 20, 2021

I fixed that too.

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.

None yet

3 participants