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

Switch to ESLint #4069

Closed
Reinmar opened this issue May 11, 2017 · 15 comments · Fixed by ckeditor/ckeditor5-engine#956
Closed

Switch to ESLint #4069

Reinmar opened this issue May 11, 2017 · 15 comments · Fixed by ckeditor/ckeditor5-engine#956
Assignees
Labels
package:engine type:task This issue reports a chore (non-production change) and other types of "todos".
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented May 11, 2017

Part of #366.

Requires: https://github.com/ckeditor/ckeditor5-dev/issues/191.

@Reinmar
Copy link
Member Author

Reinmar commented May 11, 2017

I pushed t/953 with a prototype.

It requires switching ckeditor5-dev to t/191 and running npm run switch-to-dev-dev in ckeditor5.

@pomek
Copy link
Member

pomek commented May 12, 2017

When I called gulp lint on a branch t/953 in this repository I have an error:

image

Corrected gulpfile.js for the future:

/**
 * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.
 * For licensing, see LICENSE.md.
 */

'use strict';

const gulp = require( 'gulp' );
const lintConfig = {
	// Files ignored by `gulp lint` task.
	// Files from .gitignore will be added automatically during task execution.
	ignoredFiles: [
		'src/lib/**'
	]
};

const ckeditor5Lint = require( '@ckeditor/ckeditor5-dev-lint' );

gulp.task( 'lint', () => ckeditor5Lint.lint( lintConfig ) );
gulp.task( 'lint-staged', () => ckeditor5Lint.lintStaged( lintConfig ) );
gulp.task( 'pre-commit', [ 'lint-staged' ] );

@Reinmar
Copy link
Member Author

Reinmar commented May 12, 2017

I have committed changes to ckeditor5 (symlinking) just a couple of mins ago. The same with ckeditor5-engine. Please try again.

@Reinmar
Copy link
Member Author

Reinmar commented May 12, 2017

The gulpfile.js is already updated. Config too. package.json too.

@pomek
Copy link
Member

pomek commented May 12, 2017

Now it works fine. Thx for speed helping :D

@pomek
Copy link
Member

pomek commented May 12, 2017

image

See you next year :D

@Reinmar
Copy link
Member Author

Reinmar commented May 12, 2017

Run eslint --fix src test and you'll have only 300 issues to fix manually. Make sure to review the automatic fixes and commit that separately.

@ma2ciek
Copy link
Contributor

ma2ciek commented May 12, 2017

@pomek I had a similar situation when I created a validator for jsdoc output 😄

@Reinmar
Copy link
Member Author

Reinmar commented May 15, 2017

I've been reviewing your changes, @pomek, in t/953 and just like I commented in ckeditor5-dev, issues should be fixed not muted. So:

  • no-invalid-this should be disabled. It doesn't make sense if we're any call/appy() or assigning functions to objects.
  • no-confusing-arrow should not be muted but the code should be cleaned.
  • one-var should never need to be muted. I don't understand why you ever needed to mute it because lines like ckeditor/ckeditor5-engine@900cc69#diff-9678cc8d16efd8d3fd875965f70835dcR554 look totally valid – what's wrong there? Was ESLint seriously complaining?

@Reinmar
Copy link
Member Author

Reinmar commented May 15, 2017

The rule of thumb when aligning code to the linter is that it should not require breaking the code and it should not require muting the linter too often.

If such cases happen we need to reconsider a given rule. Linter must be a help, not an enemy.

@pomek
Copy link
Member

pomek commented May 16, 2017

Short update:

  • no-invalid-this - we disabled this rule (ckeditor/ckeditor5-dev@a69bbc0)
  • no-confusing-arrow - I simplified the functions and the error does not occur
  • one-var - fixed

@pomek
Copy link
Member

pomek commented May 16, 2017

@Reinmar, could you review my changes once again?

@Reinmar
Copy link
Member Author

Reinmar commented May 16, 2017

Could you make a PR from this branch?

@pomek
Copy link
Member

pomek commented May 16, 2017

Will do it.

@pomek
Copy link
Member

pomek commented May 16, 2017

Reinmar referenced this issue in ckeditor/ckeditor5-engine May 16, 2017
Code style: Aligned the code style after switching to a more picky ESLint. Closes #953.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 11 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:task This issue reports a chore (non-production change) and other types of "todos". package:engine labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants