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.

Sign up for GitHub

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

Added eslint and fixed all errors #36

Merged
merged 7 commits into from
Mar 22, 2018
Merged

Added eslint and fixed all errors #36

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8b9d173
Added eslint and fixed all errors
rhbvkleef Mar 10, 2018
4d8a149
Moved eslint ignores
rhbvkleef Mar 10, 2018
bc141a9
Fixed some request changes
rhbvkleef Mar 11, 2018
88f55a7
Added exported clauses
rhbvkleef Mar 11, 2018
fbd4616
Fixed config error
rhbvkleef Mar 11, 2018
f8f25fe
Moved eslint configurations around and added comments
rhbvkleef Mar 13, 2018
1f454fd
Merge branch 'master' of github.com:CodingTrain/Flappy-Bird-Clone int…
rhbvkleef Mar 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions .eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
{
"env": {
"node": true,
"browser": true,
"mocha": true,
"amd": true,
"es6": true
},
"globals": {
"p5": true,
"assert": true,
"expect": true,
"sinon": true
},
"extends": [
"eslint:recommended",
"prettier",
"p5js",
"p5js/dom"
],
"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"
},
}
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
node_modules/
11 changes: 10 additions & 1 deletion bird.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

class Bird {
constructor() {
this.y = height / 2;
Expand All @@ -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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll adjust the eslintrc

this.icon,
this.x - this.width / 2,
this.y - this.height / 2,
this.width,
this.height
);
}

up() {
Expand Down
Loading