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 influx_tsm; Relates to #4098 #7289

Merged
merged 1 commit into from
Sep 13, 2016

Conversation

sdtsui
Copy link
Contributor

@sdtsui sdtsui commented Sep 11, 2016

Required for all non-trivial PRs
  • Rebased/mergable
  • Tests pass
  • CHANGELOG.md updated
  • Sign CLA (if not already signed)
Required only if applicable

You can erase any checkboxes below this note if they are not applicable to your Pull Request.

@mention-bot
Copy link

@sdtsui, thanks for your PR! By analyzing the annotation information on this pull request, we identified @joelegasse and @gunnaraasen to be potential reviewers

@sdtsui
Copy link
Contributor Author

sdtsui commented Sep 11, 2016

Refs #4098 .

@e-dard
Copy link
Contributor

e-dard commented Sep 12, 2016

@sdtsui thanks! This looks good. I just need to check one thing with the team and then I'll merge this.

@@ -18,30 +18,37 @@ type Stats struct {
TotalTime time.Duration
}

// AddPointsRead adds read points
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this docstring more useful with something like:

// AddPointsRead increments the number of read points.

Same with the other docstrings below this. They should also have periods at the end of each sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, @jsternberg, for the example. Let me know if you have other suggestions. I can do more on the weekend and will follow the same format/guidelines.

- Single commit, PR follows conventions laid out by @gouthamve in influxdata#5822
* main.go: struct field CpuFile should be CPUFile
* influx_inspect: loop equivalent to `for key := range...`
* adds comments to exported fields and consts
* fixes typo in `CHANGELOG.md`: text for influxdata#4702 now matches number
@sdtsui sdtsui force-pushed the 4098-cmd-influx_tsm branch from 607d3fb to 6471986 Compare September 13, 2016 01:36
@sdtsui
Copy link
Contributor Author

sdtsui commented Sep 13, 2016

Updated with better comments. Let me know if I can do anything else to help this get merged. Thanks!

@jsternberg
Copy link
Contributor

👍

@e-dard e-dard merged commit 28deff6 into influxdata:master Sep 13, 2016
@sdtsui sdtsui deleted the 4098-cmd-influx_tsm branch September 13, 2016 23:30
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