[FLINK-6835] Update code style documentation#63
Conversation
greghogan
left a comment
greghogan
left a comment
There was a problem hiding this comment.
This is certainly an improvement corresponding to the updated checkstyle. It seems a bit awkward that we have code style and IDE configuration on the website and in the project documentation.
| - **No redundant imports.** | ||
| - **No wildcard imports.** They can cause problems when adding to the code and in some cases even during refactoring. | ||
| - **Import order.** Imports must be ordered alphabetically, grouped into the following blocks, with each block separated by an empty line: | ||
| - <imports from other libraries> |
There was a problem hiding this comment.
Missing org.apache.flink imports.
contribute-code.md
Outdated
| ### Naming | ||
| - **Package names must start with a letter, and must not contain upper-case letters or special characters.** | ||
| - **Non-private static final fields must be upper-case, with words being separated by underscores.** (`MY_STATIC_VARIABLE`) | ||
| - **non-static fields/methods must be in lower camcel case.** (`myNonStaticField`) |
| - **Comments**. Add comments to your code. What is it doing? Add JavaDocs or inherit them by not adding any comments to the methods. Do not automatically generate comments and avoid unnecessary comments like: | ||
|
|
||
| ### Javadocs | ||
| - **All public/protected methods and classes must have a Javadoc.** |
There was a problem hiding this comment.
package private don't require javadocs; whether that is excluded in "non-private" may be up to interpretation.
contribute-code.md
Outdated
|
|
||
| ### Javadocs | ||
| - **All public/protected methods and classes must have a Javadoc.** | ||
| - **The first sentence of the javadoc must end with a dot.** |
|
Where do we reference checkstyle in the project docs? I only found this https://ci.apache.org/projects/flink/flink-docs-release-1.3/internals/ide_setup.html#checkstyle, which IMO we should also move to the website because it isn't relevant for users, which as i understand it is the target audience of the project docs. |
content/contribute-code.html
Outdated
| <li><strong>No wildcard imports.</strong> They can cause problems when adding to the code and in some cases even during refactoring.</li> | ||
| <li><strong>Import order.</strong> Imports must be ordered alphabetically, grouped into the following blocks, with each block separated by an empty line: | ||
| <ul> | ||
| <li><imports from flink packages></li> |
There was a problem hiding this comment.
Would it be clearer to state imports from org.apache.flink.*?
|
@greghogan could you take another look? |
|
@zentol @greghogan what's the status of this PR? |
|
@tzulitai IMO it's good to merge. |
|
I just got around to trying the new setup with IntelliJ and I think it's a very nice improvement compared to what we had before. +1 to merge Follow ups:
|
|
@uce We have a guide to setup checkstyle in the IDE setup section of the documentation: https://ci.apache.org/projects/flink/flink-docs-release-1.4/internals/ide_setup.html#checkstyle I think the checkstlye guide (both what we merged here and the setup guide) belong on the project website. The documentation is primarily geared towards Flink users, there's no need for a checkstyle section in there. The contributor guide on the website seems like a good place for it. |
5 participants
Updates the code style section in regards to checkstyle.
Also addresses FLINK-3814.