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

Is there a coding standards doc for Rocket.Chat? #1163

Closed
shrop opened this issue Oct 19, 2015 · 22 comments
Closed

Is there a coding standards doc for Rocket.Chat? #1163

shrop opened this issue Oct 19, 2015 · 22 comments

Comments

@shrop
Copy link

shrop commented Oct 19, 2015

Just looking for Rocket.Chat coding standards doc so my team can contribute. Thanks!

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@kakawait
Copy link
Contributor

Should create a more common document like CONTRIBUTING.md that cover coding style, processes, tricks and other useful things for developers.

👍

Actually there is just a https://github.com/RocketChat/Rocket.Chat/blob/master/.editorconfig

@gmsecrieru
Copy link
Contributor

Thanks for your interest! I think that haven't been started yet. Would you be willing to kick-off a document? There are lots of things we could standardise, maybe we could just start by tracking them.

@kakawait
Copy link
Contributor

@gmsecrieru You/We can copy CONTRIBE.md from other famous project and adapt as base

@geekgonecrazy
Copy link
Contributor

Any good examples of very well done contribute.md?

I think this is a very needed dicussion. We do need to standardize the coding style.

We shouldn't let this prevent us, but this could be effected by #1015

@kakawait
Copy link
Contributor

@geekgonecrazy yeah when I spoke about CONTRIBUTE.md is for structure/format only! You should write your own standard.

ATM I don't have sample CONTRIBUTE.md but I will check on top project (https://github.com/trending)!

@kakawait
Copy link
Contributor

My 2 cents datamining I found Angular CONTRIBUTING.md file pretty complete (but maybe too many) https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md

@shrop
Copy link
Author

shrop commented Oct 20, 2015

@gmsecrieru
Copy link
Contributor

👍 this is a great contribution!

@shrop
Copy link
Author

shrop commented Oct 22, 2015

Thanks @gmsecrieru. I am starting a Quip document that can be a start and get in to a PR as a start. Contributing.md can easily be added to and changed from there, so we just need a solid start that the project leads agree fits Rocket.Chat coding and overall philosophy.

@shrop
Copy link
Author

shrop commented Oct 25, 2015

@gmsecrieru 2 tabs for indentions is a Rocket.Chat style I can add to the doc?

@gmsecrieru
Copy link
Contributor

I would say so.

@shrop
Copy link
Author

shrop commented Oct 25, 2015

Formatting standards like this may be helpful: https://github.com/polarmobile/coffeescript-style-guide

@kakawait
Copy link
Contributor

It's my 2 cents point of view but when working with community we should create a standard documentation and above all use tools like linter (#1195), coding style checker to ensure new PR respect standard.

@gmsecrieru
Copy link
Contributor

+1

@shrop
Copy link
Author

shrop commented Oct 27, 2015

@kakawait couldn't agree more

@karlprieb
Copy link
Contributor

Any update?

@engelgabriel
Copy link
Member

Started at https://rocket.chat/docs/developer-guides/code-styleguide

@MartinSchoeler please add a NodeJS section.

@engelgabriel engelgabriel modified the milestones: 0.55.0, 0.56.0 Apr 6, 2017
@engelgabriel engelgabriel modified the milestones: 0.56.0, 0.57.0 May 9, 2017
@paulovitin
Copy link
Contributor

paulovitin commented Dec 21, 2017

@geekgonecrazy and @graywolf336 about the discussion of how to check the presence of property in object.

The bug in code is by no checking if user contains roles property, so a send a PR using a secure check of property in an object using ESLint recommendation:

if(Object.hasOwnProperty.call(user, 'roles') && Array.isArray(user.roles)) {
}

But, for not being so clear of understand and no according to code style, we agree to use like this:

if(user.roles && Array.isArray(user.roles)) {
}

But this can cause some side effects if we checking a property and this property has a false value. We need do the things most safety way possible.

So I believe this approach can be better using destructuring and checking the type of property, like this:

// this dont throw error if user not has property roles
// roles in this case will be undefined
const { roles } = user;

if(Array.isArray(roles)) {
} 

Is most readable and good styling.

@rodrigok rodrigok modified the milestones: 0.60.0, 0.61.0 Dec 21, 2017
@rodrigok
Copy link
Member

rodrigok commented Dec 22, 2017

@paulovitin IMO, there is no need to check if roles property exists and then check the property type, you could check the type directly:

if(Array.isArray(user.roles)) {
...
}

@paulovitin
Copy link
Contributor

@rodrigok you are right. 👍

@rodrigok rodrigok modified the milestones: 0.61.0, 0.61.1 Jan 22, 2018
@rodrigok rodrigok modified the milestones: 0.61.1, Short-term Feb 13, 2018
@marceloschmidt
Copy link
Member

We currently use eslint to validate coding style, and our CI will not allow PR without passing the rules. Please check https://github.com/RocketChat/eslint-config-rocketchat

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests