Add Technical and style guide to the contribution guide #2250
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tenzen-y The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
google-oss-prow
bot
requested review from
andreyvelich,
anencore94 and
johnugeorge
|
/assign @kubeflow/wg-automl-leads |
There was a problem hiding this comment.
Thank you for adding this @tenzen-y! I added a few comments.
/assign @johnugeorge
docs/developer-guide.md
Outdated
| ## Technical and style guide | ||
|
|
||
| The following guidelines apply primarily to Katib, | ||
| but other projects like training-operator might also adhere to them. |
There was a problem hiding this comment.
| but other projects like training-operator might also adhere to them. | |
| but other projects like [Training Operator](https://github.com/kubeflow/training-operator) might also adhere to them. |
|
|
||
| When coding: | ||
|
|
||
| - Follow [effective go](https://go.dev/doc/effective_go) guidelines. |
There was a problem hiding this comment.
Maybe we should add that developer should use gofmt to format their code.
There was a problem hiding this comment.
It would be better to focus on mentioning only items that it is difficult to automatically detect in CI.
WDYT?
There was a problem hiding this comment.
Maybe it makes sense to say that developer can run ./hack/verify-gofmt.sh to format their Go codes. In that case, we can reduce number of changes that contributors need to do when they create PRs.
Like in K8s developer guides: https://github.com/kubernetes/community/blob/d48200bb58cfb877e4cd77043db1e9eb6d4c8e4e/contributors/devel/development.md#presubmission-verification
There was a problem hiding this comment.
It makes sense.
As an alternative, I would suggest using make check. WDYT?
Line 31 in 4617346
There was a problem hiding this comment.
Sure, that even better, thanks!
docs/developer-guide.md
Outdated
|
|
||
| Testing: | ||
|
|
||
| - Use `cmp.Diff` instead of `reflect.Equal`, to provide useful comparisons. |
There was a problem hiding this comment.
Would be nice to provide link to cmp.Diff.
docs/developer-guide.md
Outdated
| Testing: | ||
|
|
||
| - Use `cmp.Diff` instead of `reflect.Equal`, to provide useful comparisons. | ||
| - Define test cases as maps instead of slices to avoid dependencies on te running order. |
There was a problem hiding this comment.
Maybe we should say that:
| - Define test cases as maps instead of slices to avoid dependencies on te running order. | |
| - Define test cases as maps instead of slices to avoid dependencies on the running order. Map key should be equal to the test case name. |
There was a problem hiding this comment.
That’s a great clarification!
|
|
||
| The following guidelines apply primarily to Katib, | ||
| but other projects like training-operator might also adhere to them. | ||
|
|
There was a problem hiding this comment.
Should we make a section about Golang here since Katib has various components written in Go, Python, Typescript?
| ## Golang Development | |
andreyvelich
changed the title
tenzen-y
force-pushed
the
add-coding-guide
branch
from
044d82e to
17bed61
Compare
|
@andreyvelich I addressed all your comments. PTAL |
|
I will create a separate PR to fix the Python lint error. |
I opened a PR: #2251 |
Signed-off-by: tenzen-y <[email protected]>
tenzen-y
force-pushed
the
add-coding-guide
branch
from
17bed61 to
3731cba
Compare
|
Rebased |
What this PR does / why we need it:
I added the coding guide to the contribution guide.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Part-of #2186
Checklist: