Skip to content
This repository has been archived by the owner on Jul 5, 2022. It is now read-only.

Adding a linter #496

Open
MeaningOf42 opened this issue Mar 5, 2018 · 5 comments
Open

Adding a linter #496

MeaningOf42 opened this issue Mar 5, 2018 · 5 comments

Comments

@MeaningOf42
Copy link
Contributor

As @shiffman mentioned in #493, it would be a good idea to add a linter to this site to ensure only clean code is submitted. I would be willing to give this a go, but I have some questions about his comment:

I prefer spaces around => I guess I should add some sort of linter to this repo? Is AirBnB style guide what I should adopt and is there a linter that goes with it? This is new to me, but I would love to make a video about it so advice appreciated! @meiamsome

  • As Daniel asked, would the AirBnB style guide be what we should adopt? @meiamsome

  • ESLint seems like a good linter to use with JavaScript and it a fair number of other people have documented using it to enforce the airbnb style guide therefore it seems like a good idea to use. Anyone have any ideas regarding the choice of linter we use?

  • I think we want to do the linting as part of circleci to prevent unclean code from being committed, am I right in this matter.

  • In "I guess I should add some sort of linter to this repo?" by this repo do you mean the whole website or just unit testing?

    • If you mean the whole site then you might have to change tutorial code to comply with the style guide, which would mean deviating from the code shown in the videos, would this be ok?
@meiamsome
Copy link
Member

I linked Dan to ESLint's website as my suggestion for a linter as it is my preferred JS linter. The init of ESLint lets you chose from 3 default styles or create your own. The defaults are:

  1. Google
  2. AirBnB
  3. Standard

Note that Standard disallows semicolons at the end of lines, which is very contrary to @shiffman's style as I have seen it. I could not work out if Google or AirBnB matches closer. The other option is to just create an ESLint config manually, which is not terribly hard as ESLint defaults are fairly non-controversial. We also have to note that there is a mixture of Node and Browser code in this repository if we want to lint everyting, so we need to be careful about what we disallow (Some older style code produces warnings in the standard style).

@MeaningOf42
Copy link
Contributor Author

I dislike the idea of starting a new config from scratch as that would create a new style for people to learn. However if we made sure that all our decisions are compatible with one of the style defaults then, I would be ok creating a custom config file, as we could simply tell people it is a less strict version of the style it is compatible with. (Not sure if I was clear. My problem would be if we adopted some practices from Google and some from AirBnB as this would create a confusing mixture of the two.)

I think vanilla Google or vanilla AirBnB seem like the best options, but that is just my taste. I think it goes without saying that the style we end up adopting (if we follow through with the idea of adding a lint) should be similar to @shiffman's style. Seeing as he is (obviously) the person with the most experience with and intuition of his style it would be nice to hear his thoughts on the matter before starting work on linting.

As per the node vs browser code issue, we could potentially have two different styles for the two. However I think it would be best if we didn't do this, as part of the beauty of JavaScript is being able to use the same language for front and back end development, and enforcing two different styles for the two would break this nicety. (I'm not sure if the Google or AirBnB styles would cause issues if we enforced them for server and browser code, but I don't think so).

@Versatilus
Copy link

Considering his focus on p5js, it seems logical to me that he would adopt a style similar to the one they use. In my opinion, it's the most straightforward to adopt and understand, and most importantly, to make a video about. There's a lot of homework involved in using AirBnB or Google style guides. I will include below a slightly stripped-down version of what they use for p5. He could use it as a global default, with local settings customized for specific projects. The beautify package in Atom can be set to use the eslint fixer, which should make it easy to maintain consistency

.eslintrc.js:

module.exports = {
  env: {
    browser: true,
    amd: true,
    es6: true
  },
  extends: ['eslint:recommended', 'prettier'],
  plugins: ['prettier'],
  rules: {
    'prettier/prettier': [
      'error',
      {
        singleQuote: true
      }
    ],
    'no-cond-assign': [2, 'except-parens'],
    eqeqeq: ['error', 'smart'],
    'no-use-before-define': [
      2,
      {
        functions: false
      }
    ],
    'new-cap': 0,
    'no-caller': 2,
    'no-undef': 2,
    'no-unused-vars': ['error', { args: 'none' }],
    'no-empty': ['error', { allowEmptyCatch: true }],
    'no-console': 'off'
  }
};

@shiffman
Copy link
Member

shiffman commented Mar 7, 2018

Thank you for this discussion! I have to admit I am flying a bit blind here as this is (embarrassingly) my first experience setting up my own linter. The good news is that my life's passion is indentation and spacing. I like @Versatilus of starting with the p5 style and adjusting as needed. I think it would be a good learning experience (and lead to a tutorial) if I try to set this up. I will take a look hopefully sometime this week. Any tips or further thoughts welcome!

@shiffman shiffman self-assigned this Mar 7, 2018
shiffman added a commit that referenced this issue Mar 7, 2018
This is not at all ready to be merged. I am learning about how linting
works and am starting this branch to sort it out. Some questions:

* I have a `package.json` now in the root directory. Should this be
moved to a `linting` directory or merged with
`unit_testing/package.json`?
* As suggested by @Versatilus I am starting with the p5.js
`.eslintrc.js` configuration. This can change over time.

I'll continue the discussion in the pull request comment thread.
AryanGitHub pushed a commit to AryanGitHub/Rainbow-Code that referenced this issue Apr 8, 2018
This is not at all ready to be merged. I am learning about how linting
works and am starting this branch to sort it out. Some questions:

* I have a `package.json` now in the root directory. Should this be
moved to a `linting` directory or merged with
`unit_testing/package.json`?
* As suggested by @Versatilus I am starting with the p5.js
`.eslintrc.js` configuration. This can change over time.

I'll continue the discussion in the pull request comment thread.
@jonnytest1
Copy link
Contributor

maybe would be a good idea to only allow merges / pushes when the linter isnt complaining about anything . For those that want to go full force we could also add tslint with jsdoc type annotations oO even though that would probably break the "this." meme ...

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

No branches or pull requests

7 participants