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

Add ESLint, code stye polish #1327

Merged
merged 2 commits into from Nov 6, 2015

Conversation

Projects
None yet
5 participants
@SergioCrisostomo
Member

SergioCrisostomo commented Oct 20, 2015

No description provided.

Show outdated Hide outdated Tests/eslint-configure/eslint.json
"no-trailing-spaces": 2,
"whitespace-mootools-codestyle" : 2
}
}

This comment has been minimized.

@arian

arian Oct 20, 2015

Member

Check the codestyle of this json file (indentation) ;)

@arian

arian Oct 20, 2015

Member

Check the codestyle of this json file (indentation) ;)

Show outdated Hide outdated Tests/eslint-configure/rules/no-alert.js
}
}
};
};

This comment has been minimized.

@arian

arian Oct 20, 2015

Member

Why have this rule here?

@arian

arian Oct 20, 2015

Member

Why have this rule here?

Show outdated Hide outdated Gruntfile.js
@@ -8,6 +8,14 @@ module.exports = function(grunt) {
var pullRequest = process.env.TRAVIS_PULL_REQUEST;
grunt.initConfig({
eslint: {

This comment has been minimized.

@arian

arian Oct 20, 2015

Member

tabs/spaces.

@arian

arian Oct 20, 2015

Member

tabs/spaces.

Show outdated Hide outdated Gruntfile.js
@@ -8,6 +8,14 @@ module.exports = function(grunt) {
var pullRequest = process.env.TRAVIS_PULL_REQUEST;
grunt.initConfig({
eslint: {
target: ['Source/**/*.js'],

This comment has been minimized.

@arian

arian Oct 20, 2015

Member

I'd add Gruntfile.js, Tests/**/*.js

@arian

arian Oct 20, 2015

Member

I'd add Gruntfile.js, Tests/**/*.js

@anutron

This comment has been minimized.

Show comment
Hide comment
@anutron

anutron Oct 20, 2015

Member

I'm going to get crucified for this, but it's 2015. Can we please switch to spaces for indentation now?

Member

anutron commented Oct 20, 2015

I'm going to get crucified for this, but it's 2015. Can we please switch to spaces for indentation now?

@timwienk

This comment has been minimized.

Show comment
Hide comment
@timwienk

timwienk Oct 20, 2015

Member

No.

It's 2015, there are more programmers and editors out there than ever, with more preferences than ever. No need to force a specific indentation width on anyone.

Member

timwienk commented Oct 20, 2015

No.

It's 2015, there are more programmers and editors out there than ever, with more preferences than ever. No need to force a specific indentation width on anyone.

@arian

This comment has been minimized.

Show comment
Hide comment
@arian

arian Oct 20, 2015

Member

and the ) { space too?

Member

arian commented Oct 20, 2015

and the ) { space too?

@anutron

This comment has been minimized.

Show comment
Hide comment
@anutron

anutron Oct 20, 2015

Member

Github is the normalizer here. It treats tabs as 4 spaces. Editors are smart enough that we - programmers - don't even notice which one it is. But gitub does. It makes all our lines longer than the width of the column here.

Member

anutron commented Oct 20, 2015

Github is the normalizer here. It treats tabs as 4 spaces. Editors are smart enough that we - programmers - don't even notice which one it is. But gitub does. It makes all our lines longer than the width of the column here.

Show outdated Hide outdated Gruntfile.js
eslint: {
target: ['Source/**/*.js'],
options: {
configFile: 'Tests/eslint-configure/eslint.json',

This comment has been minimized.

@arian

arian Oct 20, 2015

Member

If you use .eslintrc, the file is automatically picked up by the command line tool, which is useful for editor plugins for example.

@arian

arian Oct 20, 2015

Member

If you use .eslintrc, the file is automatically picked up by the command line tool, which is useful for editor plugins for example.

@timwienk

This comment has been minimized.

Show comment
Hide comment
@timwienk

timwienk Oct 20, 2015

Member

and the ) { space too?

That thing I never really cared about, that's been there because Valerio once upon a time decided he liked it. It's just been moo style since the start.

Github is the normalizer here.

Then perhaps Github should be configurable for this as well. I'm sure they accept Pull Requests. 😉

Editors are smart enough that we - programmers - don't even notice which one it is.

That makes no sense, in an editor you can obviously change tabspace, but no one in their right mind is going to change space-space.

Member

timwienk commented Oct 20, 2015

and the ) { space too?

That thing I never really cared about, that's been there because Valerio once upon a time decided he liked it. It's just been moo style since the start.

Github is the normalizer here.

Then perhaps Github should be configurable for this as well. I'm sure they accept Pull Requests. 😉

Editors are smart enough that we - programmers - don't even notice which one it is.

That makes no sense, in an editor you can obviously change tabspace, but no one in their right mind is going to change space-space.

@timwienk

This comment has been minimized.

Show comment
Hide comment
@timwienk

timwienk Oct 20, 2015

Member

Either way, let's leave this indentation discussion out of this PR.

Member

timwienk commented Oct 20, 2015

Either way, let's leave this indentation discussion out of this PR.

@SergioCrisostomo

This comment has been minimized.

Show comment
Hide comment
@SergioCrisostomo

SergioCrisostomo Oct 20, 2015

Member

Updated! I will send a similar PR to Core also.

Member

SergioCrisostomo commented Oct 20, 2015

Updated! I will send a similar PR to Core also.

@SergioCrisostomo

This comment has been minimized.

Show comment
Hide comment
@SergioCrisostomo

SergioCrisostomo Nov 1, 2015

Member

Can we merge this? or any review/comments/change missing?

Member

SergioCrisostomo commented Nov 1, 2015

Can we merge this? or any review/comments/change missing?

@kentaromiura

This comment has been minimized.

Show comment
Hide comment
@kentaromiura

kentaromiura Nov 6, 2015

Member

@anutron if it's what bother you, just add ?ts=2 at the end of the file you're reviewing, there are also browser extension that do this to all the files :)

Member

kentaromiura commented Nov 6, 2015

@anutron if it's what bother you, just add ?ts=2 at the end of the file you're reviewing, there are also browser extension that do this to all the files :)

kentaromiura added a commit that referenced this pull request Nov 6, 2015

@kentaromiura kentaromiura merged commit 01b2352 into mootools:master Nov 6, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@SergioCrisostomo SergioCrisostomo deleted the SergioCrisostomo:eslint branch Nov 6, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment