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

Refactor ESLint configuration. #21005

Merged
merged 2 commits into from Nov 25, 2016
Merged

Refactor ESLint configuration. #21005

merged 2 commits into from Nov 25, 2016

Conversation

bardiharborow
Copy link
Member

@bardiharborow bardiharborow commented Oct 25, 2016

In lieu of an official style guide, this takes the eslint:recommended preset, and then enables every ESLint rule that does not cause significant disruption to the codebase. It then disables the rules needed for js/tests to pass, as that follows a slightly different style. We may wish to tighten quite a bit of this later, but it's better than what we currently have.

This is one half of the fix for #20466, the other half being houndci/hound#889. This works towards #20740, but I suspect there is more to do, noting especially that ESLint does not cover all folders yet, as per #17924.

@wolfy1339
Copy link
Contributor

Hound uses the config found in the Bootstrap repo, not the new config you want to give it

@mdo mdo modified the milestone: v4.0.0-alpha.6 Oct 26, 2016
@Johann-S
Copy link
Member

I read a bit on HoundCi and I didn't found any options to lint differently JavaScript from js/tests/ folders unless to ignore them with .eslintignore see :
https://houndci.com/configuration#eslint
http://eslint.org/docs/user-guide/command-line-interface#ignoring-files-from-linting

But I don't understand why you said @bardiharborow ?

Travis will fail until #21003 is merged.

If we remove the PhantomJS Bridge nothing will change because of units tests will stay in ES5 and HoundJS will continue to warn us

@bardiharborow
Copy link
Member Author

bardiharborow commented Oct 27, 2016

@Johann-S oh, that was just because js/tests/unit/phantom.js does not pass the new ESLint rules and I didn't fix it because I was going to get it removed.

I'm going to follow up with Hound and see if we can get Hound to use the recursive.eslintrc.json discovery strategy that ESLint uses by default.

@mdo
Copy link
Member

mdo commented Nov 1, 2016

Have some conflicts here now.

I'm unsure where this and #21003 lie, so @bardiharborow and @Johann-S, I defer to you two on what get's merged when :). Happy to chat more over Slack if it'd help btw.

assert.expect(41)
var $el = $('<button>Trigger</button>')
.appendTo('#qunit-fixture')
.bootstrapTooltip({ trigger: 'click hover focus', animation: false })
var tooltip = $el.data('bs.tooltip')
var $tooltip = $(tooltip.getTipElement())

function showingTooltip() { return $tooltip.hasClass('active') || tooltip._hoverState == 'active' }
function showingTooltip() { return $tooltip.hasClass('active') || tooltip._hoverState === 'active' }

Choose a reason for hiding this comment

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

Statement inside of curly braces should be on next line brace-style
This line has 2 statements. Maximum allowed is 1 max-statements-per-line
Closing curly brace should be on the same line as opening curly brace or on the line after the previous block brace-style

@@ -608,7 +608,7 @@ $(function () {

var $tooltip = $('<a href="#" rel="tooltip" title="Another tooltip"/>')
.appendTo('#qunit-fixture')
.bootstrapTooltip({ delay: { show: 0, hide: 150 }})
.bootstrapTooltip({ delay: { show: 0, hide: 150 } })

Choose a reason for hiding this comment

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

Expected a line break after this opening brace object-curly-newline
Object properties must go on a new line object-property-newline
Expected a line break before this closing brace object-curly-newline

@@ -587,7 +587,7 @@ $(function () {

var $tooltip = $('<a href="#" rel="tooltip" title="Another tooltip"/>')
.appendTo('#qunit-fixture')
.bootstrapTooltip({ delay: { show: 150, hide: 0 }})
.bootstrapTooltip({ delay: { show: 150, hide: 0 } })

Choose a reason for hiding this comment

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

Expected a line break after this opening brace object-curly-newline
Object properties must go on a new line object-property-newline
Expected a line break before this closing brace object-curly-newline

@@ -540,7 +540,7 @@ $(function () {

var $tooltip = $('<a href="#" rel="tooltip" title="Another tooltip"/>')
.appendTo('#qunit-fixture')
.bootstrapTooltip({ delay: { show: 0, hide: 150 }})
.bootstrapTooltip({ delay: { show: 0, hide: 150 } })

Choose a reason for hiding this comment

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

Expected a line break after this opening brace object-curly-newline
Object properties must go on a new line object-property-newline
Expected a line break before this closing brace object-curly-newline

@@ -382,7 +382,7 @@ $(function () {
var $tooltip = $($target.data('bs.tooltip').tip)

// this is some dumb hack stuff because sub pixels in firefox
var top = Math.round($target.offset().top + ($target[0].offsetHeight / 2) - ($tooltip[0].offsetHeight / 2))
var top = Math.round($target.offset().top + $target[0].offsetHeight / 2 - $tooltip[0].offsetHeight / 2)

Choose a reason for hiding this comment

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

All 'var' declarations must be at the top of the function scope vars-on-top
Unexpected var, use let or const instead no-var
No magic number: 2 no-magic-numbers

@@ -1,5 +1,5 @@
$(function () {
'use strict';
'use strict'

Choose a reason for hiding this comment

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

'use strict' is unnecessary inside of modules strict

@@ -1,5 +1,5 @@
$(function () {
'use strict';
'use strict'

Choose a reason for hiding this comment

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

'use strict' is unnecessary inside of modules strict

if (type === 'js') $content.bootstrapScrollspy({ target: '.navbar', offset: 0, method: 'position' })
else if (type === 'data') $(window).trigger('load')
if (type === 'js') { $content.bootstrapScrollspy({ target: '.navbar', offset: 0, method: 'position' }) }
else if (type === 'data') { $(window).trigger('load') }

Choose a reason for hiding this comment

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

Closing curly brace does not appear on the same line as the subsequent block brace-style
This line has 2 statements. Maximum allowed is 1 max-statements-per-line
Closing curly brace should be on the same line as opening curly brace or on the line after the previous block brace-style
Statement inside of curly braces should be on next line brace-style

@@ -399,8 +402,8 @@ $(function () {
$navbar.appendTo('#qunit-fixture')
$content.appendTo('#qunit-fixture')

if (type === 'js') $content.bootstrapScrollspy({ target: '.navbar', offset: 0, method: 'position' })
else if (type === 'data') $(window).trigger('load')
if (type === 'js') { $content.bootstrapScrollspy({ target: '.navbar', offset: 0, method: 'position' }) }

Choose a reason for hiding this comment

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

This line has 2 statements. Maximum allowed is 1 max-statements-per-line
Statement inside of curly braces should be on next line brace-style
Closing curly brace should be on the same line as opening curly brace or on the line after the previous block brace-style
Expected a line break after this opening brace object-curly-newline
Object properties must go on a new line object-property-newline
Expected a line break before this closing brace object-curly-newline

!function testActiveElements() {
if (++times > 3) return done()
function testActiveElements() {
if (++times > 3) { return done() }

Choose a reason for hiding this comment

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

No magic number: 3 no-magic-numbers
This line has 2 statements. Maximum allowed is 1 max-statements-per-line
Closing curly brace should be on the same line as opening curly brace or on the line after the previous block brace-style
Statement inside of curly braces should be on next line brace-style

@@ -231,8 +231,8 @@ $(function () {
.appendTo('#qunit-fixture')
.bootstrapScrollspy({ offset: 0, target: '#navigation' })

!function testActiveElements() {
if (++times > 3) return done()
function testActiveElements() {

Choose a reason for hiding this comment

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

Expected to return a value at the end of this function consistent-return

@@ -1,5 +1,5 @@
$(function () {
'use strict';
'use strict'

Choose a reason for hiding this comment

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

'use strict' is unnecessary inside of modules strict

@@ -1,5 +1,5 @@
$(function () {
'use strict';
'use strict'

Choose a reason for hiding this comment

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

'use strict' is unnecessary inside of modules strict

@@ -7,7 +7,7 @@
*/

(function () {
'use strict';
'use strict'

Choose a reason for hiding this comment

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

'use strict' is unnecessary inside of modules strict

@@ -1,5 +1,5 @@
$(function () {
'use strict';
'use strict'

Choose a reason for hiding this comment

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

'use strict' is unnecessary inside of modules strict

@@ -1,5 +1,5 @@
$(function () {
'use strict';
'use strict'

Choose a reason for hiding this comment

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

'use strict' is unnecessary inside of modules strict

@Johann-S
Copy link
Member

Johann-S commented Nov 2, 2016

Just a question @bardiharborow is it still possible to run QUnit tests in a web browser with your PR ? Or we have to build the tests into ES5 with Babel

EDIT :
Forget my question every thing is fine here 👍

@bardiharborow
Copy link
Member Author

@mdo, we're good to go here. Travis is green. :)

@@ -608,7 +608,7 @@ $(function () {

var $tooltip = $('<a href="#" rel="tooltip" title="Another tooltip"/>')
.appendTo('#qunit-fixture')
.bootstrapTooltip({ delay: { show: 0, hide: 150 }})
.bootstrapTooltip({ delay: { show: 0, hide: 150 } })

Choose a reason for hiding this comment

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

Expected a line break after this opening brace object-curly-newline
Object properties must go on a new line object-property-newline
Expected a line break before this closing brace object-curly-newline

@@ -540,7 +540,7 @@ $(function () {

var $tooltip = $('<a href="#" rel="tooltip" title="Another tooltip"/>')
.appendTo('#qunit-fixture')
.bootstrapTooltip({ delay: { show: 0, hide: 150 }})
.bootstrapTooltip({ delay: { show: 0, hide: 150 } })

Choose a reason for hiding this comment

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

Expected a line break after this opening brace object-curly-newline
Object properties must go on a new line object-property-newline
Expected a line break before this closing brace object-curly-newline

@@ -540,7 +540,7 @@ $(function () {

var $tooltip = $('<a href="#" rel="tooltip" title="Another tooltip"/>')
.appendTo('#qunit-fixture')
.bootstrapTooltip({ delay: { show: 0, hide: 150 }})
.bootstrapTooltip({ delay: { show: 0, hide: 150 } })

Choose a reason for hiding this comment

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

Expected a line break after this opening brace object-curly-newline
Object properties must go on a new line object-property-newline
Expected a line break before this closing brace object-curly-newline

@@ -382,7 +382,7 @@ $(function () {
var $tooltip = $($target.data('bs.tooltip').tip)

// this is some dumb hack stuff because sub pixels in firefox
var top = Math.round($target.offset().top + ($target[0].offsetHeight / 2) - ($tooltip[0].offsetHeight / 2))
var top = Math.round($target.offset().top + $target[0].offsetHeight / 2 - $tooltip[0].offsetHeight / 2)

Choose a reason for hiding this comment

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

All 'var' declarations must be at the top of the function scope vars-on-top
Unexpected var, use let or const instead no-var
No magic number: 2 no-magic-numbers

@@ -382,7 +382,7 @@ $(function () {
var $tooltip = $($target.data('bs.tooltip').tip)

// this is some dumb hack stuff because sub pixels in firefox
var top = Math.round($target.offset().top + ($target[0].offsetHeight / 2) - ($tooltip[0].offsetHeight / 2))
var top = Math.round($target.offset().top + $target[0].offsetHeight / 2 - $tooltip[0].offsetHeight / 2)

Choose a reason for hiding this comment

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

All 'var' declarations must be at the top of the function scope vars-on-top
Unexpected var, use let or const instead no-var
No magic number: 2 no-magic-numbers

@@ -1,5 +1,5 @@
$(function () {
'use strict';
'use strict'

Choose a reason for hiding this comment

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

'use strict' is unnecessary inside of modules strict

@@ -1,5 +1,5 @@
$(function () {
'use strict';
'use strict'

Choose a reason for hiding this comment

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

'use strict' is unnecessary inside of modules strict

@@ -1,5 +1,5 @@
$(function () {
'use strict';
'use strict'

Choose a reason for hiding this comment

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

'use strict' is unnecessary inside of modules strict

@@ -1,5 +1,5 @@
$(function () {
'use strict';
'use strict'

Choose a reason for hiding this comment

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

'use strict' is unnecessary inside of modules strict

if (type === 'js') $content.bootstrapScrollspy({ target: '.navbar', offset: 0, method: 'position' })
else if (type === 'data') $(window).trigger('load')
if (type === 'js') { $content.bootstrapScrollspy({ target: '.navbar', offset: 0, method: 'position' }) }
else if (type === 'data') { $(window).trigger('load') }

Choose a reason for hiding this comment

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

Closing curly brace does not appear on the same line as the subsequent block brace-style
This line has 2 statements. Maximum allowed is 1 max-statements-per-line
Closing curly brace should be on the same line as opening curly brace or on the line after the previous block brace-style
Statement inside of curly braces should be on next line brace-style

if (type === 'js') $content.bootstrapScrollspy({ target: '.navbar', offset: 0, method: 'position' })
else if (type === 'data') $(window).trigger('load')
if (type === 'js') { $content.bootstrapScrollspy({ target: '.navbar', offset: 0, method: 'position' }) }
else if (type === 'data') { $(window).trigger('load') }

Choose a reason for hiding this comment

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

Closing curly brace does not appear on the same line as the subsequent block brace-style
This line has 2 statements. Maximum allowed is 1 max-statements-per-line
Closing curly brace should be on the same line as opening curly brace or on the line after the previous block brace-style
Statement inside of curly braces should be on next line brace-style

@@ -399,8 +401,8 @@ $(function () {
$navbar.appendTo('#qunit-fixture')
$content.appendTo('#qunit-fixture')

if (type === 'js') $content.bootstrapScrollspy({ target: '.navbar', offset: 0, method: 'position' })
else if (type === 'data') $(window).trigger('load')
if (type === 'js') { $content.bootstrapScrollspy({ target: '.navbar', offset: 0, method: 'position' }) }

Choose a reason for hiding this comment

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

This line has 2 statements. Maximum allowed is 1 max-statements-per-line
Statement inside of curly braces should be on next line brace-style
Closing curly brace should be on the same line as opening curly brace or on the line after the previous block brace-style
Expected a line break after this opening brace object-curly-newline
Object properties must go on a new line object-property-newline
Expected a line break before this closing brace object-curly-newline

@@ -399,8 +401,8 @@ $(function () {
$navbar.appendTo('#qunit-fixture')
$content.appendTo('#qunit-fixture')

if (type === 'js') $content.bootstrapScrollspy({ target: '.navbar', offset: 0, method: 'position' })
else if (type === 'data') $(window).trigger('load')
if (type === 'js') { $content.bootstrapScrollspy({ target: '.navbar', offset: 0, method: 'position' }) }

Choose a reason for hiding this comment

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

This line has 2 statements. Maximum allowed is 1 max-statements-per-line
Statement inside of curly braces should be on next line brace-style
Closing curly brace should be on the same line as opening curly brace or on the line after the previous block brace-style
Expected a line break after this opening brace object-curly-newline
Object properties must go on a new line object-property-newline
Expected a line break before this closing brace object-curly-newline

!function testActiveElements() {
if (++times > 3) return done()
function testActiveElements() {
if (++times > 3) { return done() }

Choose a reason for hiding this comment

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

No magic number: 3 no-magic-numbers
This line has 2 statements. Maximum allowed is 1 max-statements-per-line
Closing curly brace should be on the same line as opening curly brace or on the line after the previous block brace-style
Statement inside of curly braces should be on next line brace-style

!function testActiveElements() {
if (++times > 3) return done()
function testActiveElements() {
if (++times > 3) { return done() }

Choose a reason for hiding this comment

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

No magic number: 3 no-magic-numbers
This line has 2 statements. Maximum allowed is 1 max-statements-per-line
Closing curly brace should be on the same line as opening curly brace or on the line after the previous block brace-style
Statement inside of curly braces should be on next line brace-style

@@ -231,8 +231,8 @@ $(function () {
.appendTo('#qunit-fixture')
.bootstrapScrollspy({ offset: 0, target: '#navigation' })

!function testActiveElements() {
if (++times > 3) return done()
function testActiveElements() {

Choose a reason for hiding this comment

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

Expected to return a value at the end of this function consistent-return

@@ -231,8 +231,8 @@ $(function () {
.appendTo('#qunit-fixture')
.bootstrapScrollspy({ offset: 0, target: '#navigation' })

!function testActiveElements() {
if (++times > 3) return done()
function testActiveElements() {

Choose a reason for hiding this comment

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

Expected to return a value at the end of this function consistent-return

@@ -1,5 +1,5 @@
$(function () {
'use strict';
'use strict'

Choose a reason for hiding this comment

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

'use strict' is unnecessary inside of modules strict

@@ -1,5 +1,5 @@
$(function () {
'use strict';
'use strict'

Choose a reason for hiding this comment

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

'use strict' is unnecessary inside of modules strict

@@ -1,5 +1,5 @@
$(function () {
'use strict';
'use strict'

Choose a reason for hiding this comment

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

'use strict' is unnecessary inside of modules strict

@@ -1,5 +1,5 @@
$(function () {
'use strict';
'use strict'

Choose a reason for hiding this comment

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

'use strict' is unnecessary inside of modules strict

@@ -7,7 +7,7 @@
*/

(function () {
'use strict';
'use strict'

Choose a reason for hiding this comment

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

'use strict' is unnecessary inside of modules strict

@@ -7,7 +7,7 @@
*/

(function () {
'use strict';
'use strict'

Choose a reason for hiding this comment

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

'use strict' is unnecessary inside of modules strict

@@ -1,5 +1,5 @@
$(function () {
'use strict';
'use strict'

Choose a reason for hiding this comment

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

'use strict' is unnecessary inside of modules strict

@@ -1,5 +1,5 @@
$(function () {
'use strict';
'use strict'

Choose a reason for hiding this comment

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

'use strict' is unnecessary inside of modules strict

@@ -1,5 +1,5 @@
$(function () {
'use strict';
'use strict'

Choose a reason for hiding this comment

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

'use strict' is unnecessary inside of modules strict

@@ -1,5 +1,5 @@
$(function () {
'use strict';
'use strict'

Choose a reason for hiding this comment

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

'use strict' is unnecessary inside of modules strict

@mdo mdo merged commit c2616fb into twbs:v4-dev Nov 25, 2016
@mdo mdo mentioned this pull request Nov 25, 2016
@mdo
Copy link
Member

mdo commented Nov 25, 2016

Had a small conflict from merging #17642, but I think I fixed it right up. Merged! Thanks so much <3.

@bardiharborow bardiharborow deleted the eslint branch November 26, 2016 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Alpha 6
Needs review
Development

Successfully merging this pull request may close these issues.

None yet

5 participants