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

Refactoring influxql package to enable golint #5366

Merged
merged 1 commit into from
Feb 18, 2016

Conversation

gabelev
Copy link
Contributor

@gabelev gabelev commented Jan 15, 2016

Cleaned up the influxql package so that there are no errors when running golint. Mainly this meant adjusting and/or adding comments, but also involved changing some variable names as you can see below.

  • CHANGELOG.md updated
  • Rebased/mergable
  • Tests pass
  • Sign CLA (if not already signed)

@jwilder
Copy link
Contributor

jwilder commented Feb 10, 2016

@gabelev Can you rebase?

@gabelev
Copy link
Contributor Author

gabelev commented Feb 10, 2016

@jwilder rebased.

@@ -1406,6 +1407,7 @@ func (s *SelectStatement) validateAggregates(tr targetRequirement) error {
return nil
}

// HasDistinct is an exported method that checks if a select statment contains DISTINCT
Copy link
Contributor

Choose a reason for hiding this comment

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

You can leave off the exported method part since anything starting with a capital letter is implicitly exported.

https://golang.org/ref/spec#Exported_identifiers

@jwilder
Copy link
Contributor

jwilder commented Feb 10, 2016

A couple of small things, but looks good.

@gabelev
Copy link
Contributor Author

gabelev commented Feb 10, 2016

@jwilder: awesome, thanks. made the changes. Also added my credit to a previous merge I had in the changelog as well.

@jwilder
Copy link
Contributor

jwilder commented Feb 11, 2016

@gabelev Can you remove the is an exported method that from the those two Godocs? All set to merge after that.

@gabelev
Copy link
Contributor Author

gabelev commented Feb 11, 2016

@jwilder whoops. must have gotten caught up in the rebase. ok. done.

thanks!

@gabelev gabelev force-pushed the lint_influxql branch 3 times, most recently from 88701aa to ae3aac3 Compare February 18, 2016 15:02
@gabelev
Copy link
Contributor Author

gabelev commented Feb 18, 2016

@jwilder: is this ok to merge?

@jwilder
Copy link
Contributor

jwilder commented Feb 18, 2016

LGTM. @benbjohnson

@@ -749,6 +749,7 @@ func (s *AlterRetentionPolicyStatement) RequiredPrivileges() ExecutionPrivileges
return ExecutionPrivileges{{Admin: true, Name: "", Privilege: AllPrivileges}}
}

// FillOption represents different options for aggregare windows.
Copy link
Contributor

Choose a reason for hiding this comment

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

"aggregate"

@gabelev
Copy link
Contributor Author

gabelev commented Feb 18, 2016

@benbjohnson: Cool. I made those two comments a little more helpful/descriptive in this latest patch.

@benbjohnson
Copy link
Contributor

👍

benbjohnson added a commit that referenced this pull request Feb 18, 2016
Refactoring influxql package to enable golint
@benbjohnson benbjohnson merged commit ae7b027 into influxdata:master Feb 18, 2016
@gabelev gabelev deleted the lint_influxql branch February 18, 2016 21:25
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.

4 participants