-
Notifications
You must be signed in to change notification settings - Fork 555
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 coding.md into docs #304
Conversation
Signed-off-by: Humble Chirammal <hchiramm@redhat.com>
Signed-off-by: Humble Chirammal <hchiramm@redhat.com>
807873e
to
4607e94
Compare
README.md
Outdated
@@ -20,6 +20,12 @@ CephFS CSI plugins, see documentation in `docs/`. | |||
|
|||
For example usage of RBD and CephFS CSI plugins, see examples in `examples/`. | |||
|
|||
## Contributing to this repo | |||
|
|||
Please follow [contibution guide](https://github.com/ceph/ceph-csi/tree/csi-v1.0/docs/coding.md) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
csi-v1.0
or master
?
README.md
Outdated
## Contributing to this repo | ||
|
||
Please follow [contibution guide](https://github.com/ceph/ceph-csi/tree/csi-v1.0/docs/coding.md) | ||
and [development-guide](https://github.com/ceph/ceph-csi/tree/csi-v1.0/docs/development-guide.md) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
docs/development-guide.md
Outdated
|
||
### Workspace and repository setup | ||
|
||
1. [Download](https://golang.org/dl/) Go (>=1.9) and [install](https://golang.org/doc/install) it on your system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go version need to be 1.11.x or greater
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
docs/development-guide.md
Outdated
Run the test suite, which includes linting checks, static code check, and unit | ||
tests: | ||
|
||
`$ make tests` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
9705b42
to
91b85e8
Compare
@Madhu-1 addressed the comments. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a small nit is required
@humblec do we need to mention that we have different branches for csi-v1.0.0 and csi-v0.3.0
docs/coding.md
Outdated
``` | ||
<import standard library packages> | ||
|
||
<import ceph packages> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ceph
to ceph-csi
@humblec CI failure PTAL
|
@Madhu-1 sure. will be on it soon. |
I think we can skip this for now. |
docs/coding.md
Outdated
* Keep variable names short for variables that are local to the function. | ||
* Do not export a function or variable name outside the package until you | ||
have an external consumer for it. | ||
* Have setter or getter interfaces/methods to access/manipulate information in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to re-read this sentence four or five time before I understood what the recommendation was. Can we word this more clearly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed it :)
<import ceph-csi packages> | ||
|
||
<import third-party packages> | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really the convention here? I thought the common convention was:
- stdlib
- 3rdparty
- local
If you look at the documenatation for "goimports" for example it says:
-local string
put imports beginning with this string after 3rd-party packages; comma-separated list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, corrected it.
@phlogistonjohn PTAL .. I have addressed the comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a contributor agreement in place or call out something around the same? As we are adding contributor documentation, thinking if we need to add the same with this PR or as a follow up.
* Keep variable names short for variables that are local to the function. | ||
* Do not export a function or variable name outside the package until you | ||
have an external consumer for it. | ||
* Do not use named return values in function definitions. Use only the type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Madhu-1 and myself had a disagreement over this, I am fine with the rule being called out, but I would like to know why we should not use named returns and only use naked returns?
The answer of common style across code does not fit IMO as named returns are for short functions and not for all functions as per the language definition, hence all code cannot accommodate a single style.
README.md
Outdated
@@ -20,6 +20,12 @@ CephFS CSI plugins, see documentation in `docs/`. | |||
|
|||
For example usage of RBD and CephFS CSI plugins, see examples in `examples/`. | |||
|
|||
## Contributing to this repo | |||
|
|||
Please follow [contibution guide](https://github.com/ceph/ceph-csi/tree/master/docs/coding.md) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not call the coding.md a contribution guide, I would call it the coding style guidelines, and hence reorder the links in this statement, with dev-guide first and then coding guidelines next.
* Do not panic() for errors that can be bubbled up back to user. Use panic() | ||
only for fatal errors which shouldn't occur. | ||
* Do not ignore errors using `_` variable unless you know what you're doing. | ||
* Error strings should not start with a capital letter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linter catches this, as a result I would remove this from this list. Less rules to remember for the contributor.
* Do not ignore errors using `_` variable unless you know what you're doing. | ||
* Error strings should not start with a capital letter. | ||
* If error requires passing of extra information, you can define a new type | ||
* Error types should end with `Error`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above 2 possibly need examples
|
||
### Logging | ||
|
||
* If a function is only invoked as part of a transaction step, always use the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is a transaction? Which ID would we be using here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most updates LGTM, but I do think you should address @ShyamsundarR's comments about the logging.
@humblec anything pending on this one? |
5aba12a
to
529aeaf
Compare
Signed-off-by: Humble Chirammal <hchiramm@redhat.com>
@Madhu-1 all the comments are addressed. PTAL |
Thanks @Madhu-1 , merging this. |
@@ -1,7 +1,7 @@ | |||
# Ceph CSI 1.0.0 | |||
|
|||
[Container Storage Interface | |||
(CSI)](https://github.com/container-storage-interface/) driver, provisioner, | |||
This repo containes [Container Storage Interface(CSI)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
containes/ contains
Add coding.md into docs
Syncing latest changes from upstream devel for ceph-csi
Signed-off-by: Humble Chirammal hchiramm@redhat.com