Create a code standards styleguide

[lpatmo]

Draft: https://docs.google.com/document/d/1PGiUmTHzon16y9LsWuUKBVgDRXB6Al1p3akyuCW4DOM/edit

[angelocordon]

I think this is something where ESLint would come in to play by configuring it to work with coding style guides.

I really like Standard, but there are others such as:

• AirBnB (Rules | ESLint Config)
• Google (Rules | ESLint Config)

[angelocordon]

From @distalx:

// Type A

_.pullAllBy(array, values, [iteratee=_.identity])

// Type A
// length = 93 character;
waitinglist_applicants = _.pullAllBy(waitinglist_applicants, allocated_applicant , 'user.id');

-------------------------

// Type B 
`waitinglist_applicants = _.pullAllBy(waitinglist_applicants, 
                                    allocated_applicant , 
                                    'user.id')`

// Type B
_.pullAllBy(array, 
            values, 
            [iteratee=_.identity])

These are two different type of code blocks where “lodash” library was used.

• In lodash documentation code has been documented as a “Type A”. Now sometimes the actual usage might be logger than 80 characters. So tools like prettier will convert that code into “Type B” and if you are always accustomed to seeing code in “Type A” manner. this code modification will kinda throw you off or take you by surprise.

• Right now it’s 93 character in length. Let’s say we reduced it to 80 characters by switching the variable names. that won’t be expressive enough but that would be a one tread-off. But now if new code is a bit nested inside of some function it will also get converted to “Type B” because 80 character + tab size would be 82 or maybe 84.

• And there is a one more factor, that at one place if you have seen some API in our case “_.pullAllBy” used in a particular way, either “Type A” or “Type B”. we expect that it stays in a same format every where else. At-least I do have this expectation. So now if use of this very same API don’t violate the 80 character limit, I would have this urge to re-write as a “Type B” because for me it needs to be consistent.

• We don’t need any character limit per line at all that is not my point here, But when people come up with this standards they were bound by technological limitations that we don’t have anymore.

• I understand that not everybody has wide screen monitors, but not every single line is going to be 100 character long.

This character limit does not tell us where one should split their code, but when. For me, it make sense to allow flexibility in character per limit.

[lpatmo]

I've updated https://docs.google.com/document/d/1PGiUmTHzon16y9LsWuUKBVgDRXB6Al1p3akyuCW4DOM/edit to reflect that we're flexible on char length. Fwiw, Airbnb's character length suggestions also vary depending on the situation:

[angelocordon]

Tackling this as part of #40 --

I'm planning on integrating ESLint and to use Semi-standard as a style guide for us to follow. AirBnB is great, but has a bit of a high learning curve; I've found Standard to be the most natural for beginning level developers, and Semi-standard is all of those Standard guidelines + ensuring that we use semi-colons.

Note: I've been on and off with semi-colons, but I think I lean in more towards making sure we use it; I think in general it's good practice, instead of relying on build tools to add it on there for us. I'm not married to it though, if folks just simply prefer we don't enforce it, however we should pick one.

Do folks here know how ESLint works? Happy to demo sometime.

Essentially, a linter should warn you about code style violations as you code. They're good for catching small things like unused variables, or calling the same variable more than once, and they're also great for not having to bike shed too much on a PR.

[angelocordon]

@lpatmo okay to close this issue as #54 is merged in?

[lpatmo]

Can you share what (if any) commands developers will have to run, or if all this will be checked by Travis when a new PR is created? (Looks like Travis will do all the checks, but want to make sure.)

[angelocordon]

All of it is automated, though I can point out a few of the scripts, in case you want to run them manually before pushing a commit.

When you commit something, Husky and Lint-Staged work together to format your code with Prettier. https://github.com/codebuddies/cb-connect/blob/acafccfd201e443aedb77aed0e46a43e771e6b64/package.json#L65-L74

If you want to lint something before committing, you can run meteor npm run lint to get the reports back from ESLint. Here are other scripts you can run: https://github.com/codebuddies/cb-connect/blob/acafccfd201e443aedb77aed0e46a43e771e6b64/package.json#L9-L12

lint:fix tries to fix linting errors, prettier:check reports formatting violations. Running prettier:all will format all JS or JSX files.

Depending on your IDE, there are typically plugins that support inline linting to report to you that something isn't quite "right". For VSCode, I use the ESLint Plugin, which gives me warnings such as below: A contributor shouldn't have to set up anything other than installing the proper ESLint plugin version for their IDE. All of these rules are setup based on our ESLint configuration. For a list of all the rules, see Standard. The only thing we don't fully follow is using semicolons, which Prettier will take care for us, so if a contributor prefers not to write it, it's okay, it will be written for them anyways when they commit.

--

As for the automation aspect, when we create a PR, the branch is automatically cloned in a Travis container and it runs meteor npm lint and runs our tests. If anything errors out, it automatically fails the build and reports it back to the PR. Automated QA is nice so we can make sure that we can keep up with our coding standards, but more importantly make sure that our app is running properly (if we have good tests) before releasing to public 👍.

--

All in all, all of these commands are available, most of them are automated and shouldn't slow down a contributor, but most likely speed them up in writing better code :D

[angelocordon]

Closing this issue as I think we've discussed it in today's meeting and plenty of notes above. We can raise a new issue for other discussions.