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

Lint #4876

Merged
merged 3 commits into from
Dec 2, 2015
Merged

Lint #4876

merged 3 commits into from
Dec 2, 2015

Conversation

e-dard
Copy link
Contributor

@e-dard e-dard commented Nov 22, 2015

Supports #4098. Lints the following packages:

  • monitor;
  • services/continuous_querier ❗;
  • services/graphite ❗;
  • services/hh ❗;
  • services/httpd;
  • services/graphite; and
  • services/opentsdb.

services also contains the copier/internal package, which is generated, so that's obvious not been linted.

Packages with ❗ have outstanding lint issues. Specifically:

continuous_querier/config.go:1:1: don't use an underscore in package name
continuous_querier/service.go:1:1: don't use an underscore in package name
continuous_querier/service_test.go:1:1: don't use an underscore in package name
graphite/parser.go:192:78: exported func NewTemplate returns unexported type *graphite.template, which can be annoying to use
hh/limiter.go:15:34: exported func NewRateLimiter returns unexported type *hh.limiter, which can be annoying to use

Because the tests in these packages use the exported test package idiom, the NewX functions cannot be unexported as it stands. I'm not sure whether they're supposed to be part of the external API or not regardless.

In terms of the continuous_querier package I can rename this if it suits—depends on if it's name is underscored for a reason.

If anyone wants the remaining issues resolved: chime up, otherwise these packages are done.

@@ -157,8 +159,8 @@ func (p *Parser) Parse(line string) (models.Point, error) {
return models.NewPoint(measurement, tags, fieldValues, timestamp)
}

// Apply extracts the template fields form the given line and returns the
// measurement name and tags
// ApplyTemplate extracts the template fields form the given line and
Copy link
Contributor

Choose a reason for hiding this comment

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

form -> from

@otoolep
Copy link
Contributor

otoolep commented Nov 23, 2015

Thanks @e-dard -- some minor feedback. I can merge once addressed.

@e-dard
Copy link
Contributor Author

e-dard commented Nov 23, 2015

Thanks @otoolep. Sorry about the typos.

"I'll just lint this package on a Sunday evening, it'll only take five minutes"... Thirty minutes later.. Arggh!

@e-dard
Copy link
Contributor Author

e-dard commented Dec 2, 2015

Sorry for the late catchup on this; my daughter decided to arrive 3 weeks early 👶 Should be all good now @otoolep ?

@otoolep
Copy link
Contributor

otoolep commented Dec 2, 2015

Looks good to me @e-dard -- thanks. +1

@corylanou ? Please merge if you're OK with this.

corylanou added a commit that referenced this pull request Dec 2, 2015
@corylanou corylanou merged commit 3cd8056 into influxdata:master Dec 2, 2015
corylanou added a commit that referenced this pull request Dec 2, 2015
changelog entry for #4876
@e-dard e-dard deleted the lint branch January 20, 2016 10:50
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