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
Feature - 2910 Add coding guidelines #559
Conversation
Rules: | ||
- Indentation is done with 2 spaces in most cases. | ||
- Line breaks are only done if needed, try to use the 80 character limit. | ||
- Strings are splitted at the latest possible position. |
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.
split
@Poltergeist Thanks a ton for taking care of this. Really appreciated. |
## JSDoc | ||
If there is a return there should be a type and description. | ||
If there is one or multiple parameter/s there should be a name, type and description for each one. | ||
|
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 add, that the function should be understandable without commenting, so it actually needs no comment header. A function should be descriptive by reading the code or the function definition itself.
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.
If the code itself needs commenting then you probably did something wrong.
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 compare the JSDoc description to a cover text of a book you should not need to read it to understand the contents but if you scan it you know something about the contents before reading it.
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.
Think about the function-naming and the parameters as the cover text. :)
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 I find useful is to document a whole file with a meaningful description on the top, but not single functions.
This is already very complete! Great! Good work so far. The really important things are all covered. It's a good doc that can be complemented later, if needed. Some things I think about, that should be documented, are:
|
c8efae6
to
586bc57
Compare
- Indentation is 2 spaces. | ||
- No multiple empty lines. | ||
|
||
We prefer a functional approach and try to use the abstractions over |
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.
Sounds perfect to me. Thanx!
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.
Minor:
try to use the_se_ abstractions
``` | ||
|
||
## JSDoc | ||
JSDoc blocks should be avoided in favor to code descriptiveness. If the are necessary these rules apply: |
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.
👍
Little typo (maybe mine ;) ):
If the_y_
358f0f7
to
0ff251d
Compare
Remove typos Remove uneccessary examples add new examples
This change is so that the build does not break until all new rules are refactored
3d6bbea
to
d96f2ff
Compare
Let's merge this! :) This was a lot of work @Poltergeist , good job, thanx! Most things are covered! |
Feature - 2910 Add coding guidelines
For now these rules are open for discussion.
This PR introduces new eslint settings which are only warnings for now until all issues are resolved then these warnings are switched to errors.
This closes mesosphere/marathon#2910