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

add documentation and golint #17

Merged
merged 7 commits into from
Apr 28, 2020
Merged

add documentation and golint #17

merged 7 commits into from
Apr 28, 2020

Conversation

pallavJha
Copy link
Contributor

@pallavJha pallavJha commented Apr 26, 2020

@eatonphil
Copy link
Owner

Thanks! This is going to conflict with a PR I have in progress (#15). So I'll commit to this branch after I merge that PR.

I don't find it very useful to have comments that are no more explanative than the struct or function name (e.g. SelectStatement, statementKind, etc.) so I will remove those. But other than that there are some good additions here.

@eatonphil
Copy link
Owner

I see now that golint requires all these to be documented. In that case, fine.

@eatonphil
Copy link
Owner

Ok, after looking into things a little bit more I decided to go with just golangci-lint for now which is a little more configurable and I've used it in the past.

Some of these comments could still be useful, but I'd leave that up to you.

If you'd like to get the PR approved I'd say:

  1. Drop the golint
  2. Remove any comments that are super obvious

Thanks for the effort!

pallav.jha added 3 commits April 27, 2020 19:45
@pallavJha
Copy link
Contributor Author

pallavJha commented Apr 27, 2020

Hi @eatonphil

I've removed the golint and tested with the golangci-lint after resolving conflicts and merging with the master.
I've also removed some of the comments that I had added for just because of golint.
You can review it once again.

@eatonphil
Copy link
Owner

Made a few changes but looks good. Thank you!

@eatonphil eatonphil merged commit eb43261 into eatonphil:master Apr 28, 2020
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.

Add the code comments, documetation and golint
2 participants