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

Migrate to AirBnB style guide. #8315

Closed
cjcenizal opened this issue Sep 16, 2016 · 7 comments
Closed

Migrate to AirBnB style guide. #8315

cjcenizal opened this issue Sep 16, 2016 · 7 comments
Assignees

Comments

@cjcenizal
Copy link
Contributor

cjcenizal commented Sep 16, 2016

Let's agree to migrate our codebase to the AirBnB style guide (continuation of #7435 (comment), related to #4949).

Why?

This will reduce the amount of rules people need to consider when writing/reviewing code, give us a clear end goal to move our code towards, and give us a clear baseline around which we can discuss specific deviations/customizations.

The process

Extend eslint-config-airbnb

  1. Update eslint-config-kibana's package.json to have eslint-config-airbnb as a dependency ("devDependencies": { "eslint-config-airbnb": "12.0.0" }).
  2. Add extends: 'airbnb' to index.js.
  3. Test the linting task in Kibana to make sure the codebase passes the new rules successfully. If it fails, add additional rule overrides until it does pass successfully.
  4. Document which rules are overridden in the eslint-config-kibana README and link to the AirBnB style guide for reference.

Update the JS style guide docs

  1. Make a note that we are moving towards automating our style guide enforcement and link to the AirBnB style guide for reference.
  2. Add another note that when we deviate from the AirBnB style guide, we'll document these deviations in our linter config repo, and add a link to eslint-config-kibana.
  3. Edit our JS style guide and remove any rules which are made redundant by the AirBnB style guide.
  4. Any rules which can be automatically enforced by the linter should be moved from the JS style guide into the eslint-config-kibana README.

Introduce AirBnB's rules one by one

  1. Remove one of the eslint-config-kibana rule overrides to accept the AirBnB default.
  2. Use codeshift to make the code compliant for this "new" single rule (similar to @spalger 's PR that introduces the no-unused-vars rule in Upgrade eslint #8101).
  3. Run automated tests and do a manual QA pass.
  4. Repeat for additional rules until the entire codebase is updated and compliant with the AirBnB style guide.

Creating custom rules

For rules which seem controversial or objectively sub-par, conduct an analysis of the pros and cons of the rule and viable alternatives, and generate a team discussion to arrive at a decision about whether to adopt the original or a customized version of the rule.

Custom rules

  1. Prefer named exports instead of prefer default export (AirBnb style guide deviation suggestion - use named exports #8641)
@epixa
Copy link
Contributor

epixa commented Oct 3, 2016

I strongly support this change, though I think this is only reasonable if we also apply the change to 4.6, which is a lot riskier. In any case though, I think this is the right move going forward.

I would also be open to other suggestions besides the AirBnB styleguide if anyone has them, but at this point I'm not aware of any styleguide that has the same critical mass as that one, and our codebase is already similar in a lot of important ways.

@cjcenizal cjcenizal self-assigned this Oct 10, 2016
@spalger spalger removed the discuss label Oct 10, 2016
@stacey-gammon
Copy link
Contributor

@epixa, why do you think feel so strongly that we apply the change to 4.6 as well? Is it because it will be difficult for developers to adhere to two different styles depending on which repo they are editing in?

Since we recently decided to back port critical bug fixes only (which, unless I misunderstood, that includes 4.6 as well as 5.0?) then this doesn't seem critical to me.

@epixa
Copy link
Contributor

epixa commented Oct 12, 2016

@stacey-gammon I agree with you. The situation has changed since I made that comment

@epixa epixa added the chore label Oct 12, 2016
@stacey-gammon
Copy link
Contributor

stacey-gammon commented Nov 7, 2016

Did we agree on moving forward with this? If not, does anyone remember what the blocker was? More discussion or something else?

I'm excited about the prospect of this change so would be interested in starting to go through the steps @cjcenizal mentioned above, as long as it has been mutually agreed upon.

@stacey-gammon stacey-gammon self-assigned this Nov 7, 2016
@epixa
Copy link
Contributor

epixa commented Nov 7, 2016

Yep, we do want to move forward with this.

We probably need to finish our eslint upgrade process that @spalger began, which he's going to be working on soonish. It's a big change that is going to break a bunch of PRs, so he's going to figure out the extent of the damage in that regard before we rush forward, but once that is in motion it will be turned around very quickly.

@cjcenizal cjcenizal removed the discuss label Nov 7, 2016
@spalger spalger mentioned this issue Dec 2, 2016
@stacey-gammon stacey-gammon added Team:Operations Team label for Operations Team discuss labels Aug 8, 2018
@stacey-gammon
Copy link
Contributor

I'm keeping this one around, I tagged @elastic/kibana-operations. It's old but still relevant I think.

@tylersmalley
Copy link
Contributor

This issue is quite old, and there seems to be very little desire to change our style guide. If that sentiment changes, we can raise this discussion among the teams.

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

5 participants