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

[Coding guideline] Add implementation and grouping section #641

Merged
merged 14 commits into from
Aug 27, 2020

Conversation

kevindiu
Copy link
Contributor

@kevindiu kevindiu commented Aug 25, 2020

Description:

This PR update the coding guideline and create 2 new section to cover the following things:

  • add example of implementation for functional option
  • grouping type declaration
  • order of declaration

Please refer to this task ticket for this PR:
https://github.com/vdaas/vald/projects/3#card-44245759

Related Issue:

How Has This Been Tested?:

Environment:

  • Go Version: 1.14.4
  • Docker Version: 19.03.8
  • Kubernetes Version: 1.18.2
  • NGT Version: 1.12.0

Types of changes:

  • Bug fix [type/bug]
  • New feature [type/feature]
  • Add tests [type/test]
  • Security related changes [type/security]
  • Add documents [type/documentation]
  • Refactoring [type/refactoring]
  • Update dependencies [type/dependency]
  • Update benchmarks and performances [type/bench]
  • Update CI [type/ci]

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Checklist:

  • I have read the CONTRIBUTING document.
  • I have checked open Pull Requests for the similar feature or fixes?
  • I have added tests and benchmarks to cover my changes.
  • I have ensured all new and existing tests passed.
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the documentation accordingly.

@pull-assistant
Copy link

pull-assistant bot commented Aug 25, 2020

Score: 0.88

Best reviewed: commit by commit


Optimal code review plan (2 commits squashed)

     update coding guideline

     apply suggestion

     fix

     fix

     fix

fix ... apply suggestion

Squashed 2 commits:

     fix

     fix

     fix

     fix

     fix

     fix

     fix

Powered by Pull Assistant. Last update 95746ac ... 45ff9ba. Read the comment docs.

@vdaas-ci
Copy link
Collaborator

[CHATOPS:HELP] ChatOps commands.

  • 🙆‍♀️ /approve - approve
  • 💌 /changelog - add changelog comment
  • 🍱 /format - format codes and add licenses
  • /gen-test - generate test codes
  • 🏷️ /label - add labels
  • /rebase - rebase master

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

LanguageTool

docs/contributing/coding-style.md|460 col 8| Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION[1])
Suggestions: ``
Rule: https://community.languagetool.org/rule/show/UNLIKELY_OPENING_PUNCTUATION?lang=en-US&subId=1
Category: PUNCTUATION
docs/contributing/coding-style.md|462 col 3| Don't put a space before the closing parenthesis (COMMA_PARENTHESIS_WHITESPACE)
Suggestions: }
Rule: https://community.languagetool.org/rule/show/COMMA_PARENTHESIS_WHITESPACE?lang=en-US
Category: TYPOGRAPHY

docs/contributing/coding-style.md Show resolved Hide resolved
docs/contributing/coding-style.md Show resolved Hide resolved
docs/contributing/coding-style.md Show resolved Hide resolved
docs/contributing/coding-style.md Show resolved Hide resolved
docs/contributing/coding-style.md Show resolved Hide resolved
docs/contributing/coding-style.md Show resolved Hide resolved
docs/contributing/coding-style.md Show resolved Hide resolved
docs/contributing/coding-style.md Show resolved Hide resolved
docs/contributing/coding-style.md Show resolved Hide resolved
docs/contributing/coding-style.md Show resolved Hide resolved
docs/contributing/coding-style.md Outdated Show resolved Hide resolved
docs/contributing/coding-style.md Outdated Show resolved Hide resolved
docs/contributing/coding-style.md Outdated Show resolved Hide resolved
docs/contributing/coding-style.md Outdated Show resolved Hide resolved
docs/contributing/coding-style.md Show resolved Hide resolved
docs/contributing/coding-style.md Show resolved Hide resolved
docs/contributing/coding-style.md Show resolved Hide resolved
docs/contributing/coding-style.md Show resolved Hide resolved
hlts2
hlts2 previously approved these changes Aug 25, 2020
Copy link
Contributor

@hlts2 hlts2 left a comment

Choose a reason for hiding this comment

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

LGTM

docs/contributing/coding-style.md Outdated Show resolved Hide resolved
docs/contributing/coding-style.md Outdated Show resolved Hide resolved
docs/contributing/coding-style.md Outdated Show resolved Hide resolved
docs/contributing/coding-style.md Outdated Show resolved Hide resolved
docs/contributing/coding-style.md Outdated Show resolved Hide resolved
docs/contributing/coding-style.md Outdated Show resolved Hide resolved
Copy link
Contributor

@hlts2 hlts2 left a comment

Choose a reason for hiding this comment

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

LGTM

docs/contributing/coding-style.md Outdated Show resolved Hide resolved
hlts2
hlts2 previously approved these changes Aug 26, 2020
Copy link
Contributor

@hlts2 hlts2 left a comment

Choose a reason for hiding this comment

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

LGTMed

@kevindiu kevindiu requested a review from vankichi August 26, 2020 07:34
vankichi
vankichi previously approved these changes Aug 26, 2020
Copy link
Contributor

@vankichi vankichi left a comment

Choose a reason for hiding this comment

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

LGTM

@kevindiu kevindiu dismissed stale reviews from vankichi and hlts2 via d360c59 August 26, 2020 08:12
vankichi
vankichi previously approved these changes Aug 26, 2020
```

Instead of this style.
addr := conn.LocalAddr()
Copy link
Contributor

Choose a reason for hiding this comment

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

[LanguageTool] reported by reviewdog 🐶
Add a space between sentences (SENTENCE_WHITESPACE)
Suggestions: LocalAddr
Rule: https://community.languagetool.org/rule/show/SENTENCE_WHITESPACE?lang=en-US
Category: TYPOGRAPHY

rinx
rinx previously approved these changes Aug 26, 2020
hlts2
hlts2 previously approved these changes Aug 27, 2020
Copy link
Contributor

@hlts2 hlts2 left a comment

Choose a reason for hiding this comment

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

sorry I overlooked this
LGTMed

Comment on lines 384 to 401
<table>
<thead><tr><th>Bad</th><th>Good</th></tr></thead>
<tbody><tr><td>

```go
// good
if err := fn(); err != nil {
// handle error
}
```

Instead of this style.
</td><td>

```go
// bad
err := fn()
if err != nil {
// handle error
}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

opposite

@kpango
Copy link
Collaborator

kpango commented Aug 27, 2020

/rebase
/format

@vdaas-ci
Copy link
Collaborator

[REBASE] Rebase triggered by kpango for branch: documentation/coding_guideline/func_option_and_grouping

@vdaas-ci vdaas-ci force-pushed the documentation/coding_guideline/func_option_and_grouping branch from 73eab54 to 4fec1e9 Compare August 27, 2020 03:05
@vdaas-ci
Copy link
Collaborator

[FORMAT] Updating license headers and formatting go codes triggered by kpango.

// good
if err := fn(); err != nil {
err := fn()
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

[LanguageTool] reported by reviewdog 🐶
Don't put a space after the opening parenthesis (COMMA_PARENTHESIS_WHITESPACE)
Suggestions: {
Rule: https://community.languagetool.org/rule/show/COMMA_PARENTHESIS_WHITESPACE?lang=en-US
Category: TYPOGRAPHY

// bad
err := fn()
if err != nil {
if err := fn(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

[LanguageTool] reported by reviewdog 🐶
Don't put a space after the opening parenthesis (COMMA_PARENTHESIS_WHITESPACE)
Suggestions: {
Rule: https://community.languagetool.org/rule/show/COMMA_PARENTHESIS_WHITESPACE?lang=en-US
Category: TYPOGRAPHY

Copy link
Collaborator

@kpango kpango left a comment

Choose a reason for hiding this comment

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

LGTM

@kpango kpango merged commit cfcf27a into master Aug 27, 2020
@kpango kpango deleted the documentation/coding_guideline/func_option_and_grouping branch August 27, 2020 03:13
// good
conn, err := net.Dial("tcp", "localhost:80")
if err != nil {
if conn, err := net.Dial("tcp", "localhost:80"); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

[LanguageTool] reported by reviewdog 🐶
Add a space between sentences (SENTENCE_WHITESPACE)
Suggestions: Dial
Rule: https://community.languagetool.org/rule/show/SENTENCE_WHITESPACE?lang=en-US
Category: TYPOGRAPHY

// good
conn, err := net.Dial("tcp", "localhost:80")
if err != nil {
if conn, err := net.Dial("tcp", "localhost:80"); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

[LanguageTool] reported by reviewdog 🐶
Possible typo: you repeated a whitespace (WHITESPACE_RULE)
Suggestions:
Rule: https://community.languagetool.org/rule/show/WHITESPACE_RULE?lang=en-US
Category: TYPOGRAPHY

// good
conn, err := net.Dial("tcp", "localhost:80")
if err != nil {
if conn, err := net.Dial("tcp", "localhost:80"); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

[LanguageTool] reported by reviewdog 🐶
Don't put a space after the opening parenthesis (COMMA_PARENTHESIS_WHITESPACE)
Suggestions: {
Rule: https://community.languagetool.org/rule/show/COMMA_PARENTHESIS_WHITESPACE?lang=en-US
Category: TYPOGRAPHY

// handle error
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

[LanguageTool] reported by reviewdog 🐶
Don't put a space after the opening parenthesis (COMMA_PARENTHESIS_WHITESPACE)
Suggestions: {
Rule: https://community.languagetool.org/rule/show/COMMA_PARENTHESIS_WHITESPACE?lang=en-US
Category: TYPOGRAPHY

// handle error
} else {
// use the conn
Copy link
Contributor

Choose a reason for hiding this comment

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

[LanguageTool] reported by reviewdog 🐶
After 'the', do not use a verb. Make sure that the spelling of 'conn' is correct. If 'conn' is the first word in a compound adjective, use a hyphen between the two words. Note: This error message can occur if you use a verb as a noun, and the word is not a noun in standard English. (A_INFINITIVE[1])
Rule: https://community.languagetool.org/rule/show/A_INFINITIVE?lang=en-US&subId=1
Category: GRAMMAR

// handle error
} else {
// use the conn
Copy link
Contributor

Choose a reason for hiding this comment

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

[LanguageTool] reported by reviewdog 🐶
Don't put a space before the closing parenthesis (COMMA_PARENTHESIS_WHITESPACE)
Suggestions: }
Rule: https://community.languagetool.org/rule/show/COMMA_PARENTHESIS_WHITESPACE?lang=en-US
Category: TYPOGRAPHY


```go
// bad
if conn, err := net.Dial("tcp", "localhost:80"); err != nil {
conn, err := net.Dial("tcp", "localhost:80")
Copy link
Contributor

Choose a reason for hiding this comment

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

[LanguageTool] reported by reviewdog 🐶
Add a space between sentences (SENTENCE_WHITESPACE)
Suggestions: Dial
Rule: https://community.languagetool.org/rule/show/SENTENCE_WHITESPACE?lang=en-US
Category: TYPOGRAPHY

// bad
if conn, err := net.Dial("tcp", "localhost:80"); err != nil {
conn, err := net.Dial("tcp", "localhost:80")
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

[LanguageTool] reported by reviewdog 🐶
Don't put a space after the opening parenthesis (COMMA_PARENTHESIS_WHITESPACE)
Suggestions: {
Rule: https://community.languagetool.org/rule/show/COMMA_PARENTHESIS_WHITESPACE?lang=en-US
Category: TYPOGRAPHY

}
// use the conn
Copy link
Contributor

Choose a reason for hiding this comment

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

[LanguageTool] reported by reviewdog 🐶
After 'the', do not use a verb. Make sure that the spelling of 'conn' is correct. If 'conn' is the first word in a compound adjective, use a hyphen between the two words. Note: This error message can occur if you use a verb as a noun, and the word is not a noun in standard English. (A_INFINITIVE[1])
Rule: https://community.languagetool.org/rule/show/A_INFINITIVE?lang=en-US&subId=1
Category: GRAMMAR

@vdaas-ci vdaas-ci mentioned this pull request Sep 2, 2020
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants