Added eslint and fixed all errors #36

rhbvkleef · 2018-03-10T18:47:15Z

Closes #3

This PR adds eslint, and fixes the errors discovered by it. The preset used is based of the one used by the P5.js project, but has some modifications. Here is a non-exhaustive list of rules added:

allow-parens: In arrow functions, don't add parentheses with 1 argument, always add them otherwise.
brace-style: Opening brace on same line as relevant keyword, closing one on the same line of next relevant keyword, if there is any. (e.g. else)
comma-dangle: In multi-line arrays, objects, ...: the last value requires a comma behind it.
keyword-spacing: spaces around all keywords
max-len: Maximum line-length is 100 chars
max-params: Maximum parameters on one line is 6
no-cond-assign: Don't allow assignment of variable inside conditional, unless an extra pair of brackets is added.
eqeqeq: In most comparisons, require a triple equals
no-use-before-define: Require a variable defenition to happen before usage (except for functions)
new-cap: Require class names and constructor names to be capitalized. ("Bird", not "bird")
no-caller: Disallow arguments.caller and arguments.callee
no-undef: Don't allow usage of undefined variables
no-unused-vars: Don't allow defining and assigning of unused variables
no-empty: Don't allow empty code blocks
no-console: Don't allow console statements (console.log)
quotes: Require single quotes
semi: Require trailing semicolons
space-before-blocks: Require whitespace before opening curly brace

In addition to that, "eslint:recommended" adds all rules that have a checkmark in fromt of them on
this (https://eslint.org/docs/rules/) page and p5 and p5/dom add globals exported by P5 and P5.dom.

shiffman

Thank you so much for submitting this!

shiffman · 2018-03-10T23:03:32Z

bird.js

@@ -3,6 +3,9 @@
 // http://patreon.com/codingtrain
 // Code for: https://youtu.be/cXgA1d_E-jY

+/* global birdSprite */
+
+// eslint-disable-next-line no-unused-vars


Is there a way to do this in a config file? I would like the code to have minimal comments to reduce any noise as I use it as the basis for the neuro-evolution tutorials.

I had been searching for a while, a possibility is to move everything into a global comment. I am currently looking at whether it can be hidden in the .eslintrc (at which point I'll run a base configuration for you to extend) but haven't found that yet.

Apart from the fact that I think that the global asset thing is poor code-style, I don't think eslint has an easy way to configure usage checking globally. (using "no-unused-vars": ["error", {"vars":'local"}]) would be an option but that would miss a lot of critical linting errors. In the commit that I am pushing right now, I also replaced the disable-next-line clauses with exported clauses, as they are a bit clearer.

I can live with the globals though I would probably prefer to put them in a config file and not have anything related to eslint in the code itself. But if having these comments is standard JavaScript practice then maybe it's good to have that here and expose viewers to this option when linting! What about also adding something that says:

/* comments for eslint configuration */

There's probably a better way to phrase that. What do you think @meiamsome?

We can add globals to the config file for simplicity in this case, but I am not sure what is the best course of action.

I'd like to try that unless anyone objects! @rhbvkleef do you want to add that to this pul request? I'm also happy to merge and refactor after if you prefer.

Even though I personally disagree with configurations like this, I added them as you requested.

shiffman · 2018-03-10T23:04:00Z

bird.js

@@ -19,7 +22,13 @@ class Bird {

  show() {
    // draw the icon CENTERED around the X and Y coords of the bird object
-    image(this.icon, this.x - (this.width / 2), this.y - (this.height / 2), this.width, this.height);
+    image(


My preferred style is to actually leave this in one line of code. (Don't hate me!)

I'll adjust the eslintrc

shiffman · 2018-03-10T23:04:25Z

pipe.js

@@ -18,9 +21,15 @@ class Pipe {
  }

  hits(bird) {
-    if (bird.y - (bird.height / 2) < this.top || bird.y + (bird.height / 2) > this.bottom) {
+    if (


same here, one line.

shiffman · 2018-03-10T23:04:51Z

sketch.js

-  pipePeakSprite = loadImage("./graphics/pipe_body.png");
-  birdSprite = loadImage("graphics/train.png");
-  bgImg = loadImage("graphics/background.png");
+  pipeBodySprite = loadImage('./graphics/pipe_body.png');


yay single quotes! ❤️

shiffman · 2018-03-10T23:05:08Z

sketch.js

@@ -38,9 +45,9 @@ function draw() {
  // second copy of the image right next to it
  // once the second image gets to the 0 point, we can reset bgX to
  // 0 and go back to drawing just one image.
-  if(bgX <= -bgImg.width + width){
+  if (bgX <= -bgImg.width + width) {


yay spaces! 😍

I have personally never understood spaces after if/for/when - the brackets are a part of the keyword construct so why would they be separated? It would be like having function calls as loadImage ('...');, which, although syntactically valid, I have never seen anyone use.

(Note this isn't a request to change, I'm just wondering what the reasoning behind this is)

Haha, interesting point! In my mind the space actually distinguishes the structure from a function call -- if, for, and while being special control structures? But this may just be what I tell myself to sleep at night as the missing space sends me into a panic spiral.

In my reasoning, a branch construct has three or more parts, which are space separated: the keyword, the condition and the code-block. Additionally an else/elseif can be added. For me this sounds like reason to add spacing.

Ah, interesting. Those both sound like good reasoning. I think I'd be more likely to also be for that space convention if the form of if a b else c was allowed without the brackets (See Rust for an example when I think the syntax feels very intuitive). With them as required it feels to me like they should be attached, but thanks for telling me your thoughts!

rhbvkleef · 2018-03-11T12:51:01Z

When merging this, I recommend squashing. The reason is that this branch is build up from a lot of small commits, some of which don't really have descriptive names.

…o 3-add-linting-new

rhbvkleef · 2018-03-14T20:50:58Z

@shiffman Still planning on merging this or close some discussions that we had. I believe they are resolved as of my last commit.

Versatilus · 2018-03-22T22:41:39Z

It looks like everything is in order. Awesome!

rhbvkleef added 2 commits March 10, 2018 19:45

Added eslint and fixed all errors

8b9d173

Moved eslint ignores

4d8a149

shiffman requested changes Mar 10, 2018

View reviewed changes

rhbvkleef added 3 commits March 11, 2018 12:10

Fixed some request changes

bc141a9

Added exported clauses

88f55a7

Fixed config error

fbd4616

rhbvkleef mentioned this pull request Mar 11, 2018

Add linting! #3

Closed

rhbvkleef added 2 commits March 13, 2018 14:50

Moved eslint configurations around and added comments

f8f25fe

Merge branch 'master' of github.com:CodingTrain/Flappy-Bird-Clone int…

1f454fd

…o 3-add-linting-new

Versatilus merged commit b1e95b4 into CodingTrain:master Mar 22, 2018

rhbvkleef deleted the 3-add-linting-new branch March 25, 2018 12:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added eslint and fixed all errors #36

Added eslint and fixed all errors #36

rhbvkleef commented Mar 10, 2018 •

edited

Loading

shiffman left a comment

shiffman Mar 10, 2018

rhbvkleef Mar 11, 2018

rhbvkleef Mar 11, 2018

shiffman Mar 11, 2018

meiamsome Mar 11, 2018

shiffman Mar 12, 2018

rhbvkleef Mar 13, 2018

shiffman Mar 10, 2018

rhbvkleef Mar 11, 2018

shiffman Mar 10, 2018

shiffman Mar 10, 2018

shiffman Mar 10, 2018

meiamsome Mar 11, 2018

shiffman Mar 11, 2018 •

edited

Loading

rhbvkleef Mar 11, 2018

meiamsome Mar 11, 2018

rhbvkleef commented Mar 11, 2018

rhbvkleef commented Mar 14, 2018

Versatilus commented Mar 22, 2018

Added eslint and fixed all errors #36

Added eslint and fixed all errors #36

Conversation

rhbvkleef commented Mar 10, 2018 • edited Loading

shiffman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiffman Mar 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhbvkleef commented Mar 11, 2018

rhbvkleef commented Mar 14, 2018

Versatilus commented Mar 22, 2018

rhbvkleef commented Mar 10, 2018 •

edited

Loading

shiffman Mar 11, 2018 •

edited

Loading