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

Jump to bottom

Setup linter for HTML #5195

Merged
merged 41 commits into from Feb 3, 2018
Merged

Setup linter for HTML #5195

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
3ff10ac
Add htmllint
loicbourgois Jan 27, 2018
13cd5b0
Add htmllint
loicbourgois Jan 27, 2018
fddb06f
Setup html plugin for eslint
loicbourgois Jan 28, 2018
6f821d7
Add option to lint script tags
loicbourgois Jan 28, 2018
83822eb
Rename option
loicbourgois Jan 28, 2018
fd5232a
Convert spaces to tabs
loicbourgois Jan 28, 2018
7cfcf5a
Setup eslint fix
loicbourgois Jan 28, 2018
1b591b1
setup
loicbourgois Jan 28, 2018
9f130a2
Autofix eslint
loicbourgois Jan 28, 2018
c640e69
Rollback before autofix
loicbourgois Jan 28, 2018
098c133
Remove most errors
loicbourgois Jan 28, 2018
71792f1
Correct introduced errors
loicbourgois Jan 28, 2018
70822cf
Check samples
loicbourgois Jan 28, 2018
f8f119a
Fix lint
loicbourgois Jan 28, 2018
8183537
Add TODO comments
loicbourgois Jan 28, 2018
0171194
revert window.Samples
loicbourgois Jan 30, 2018
f588414
Move config to samples/.eslintrc.yml
loicbourgois Jan 30, 2018
599fd09
Enforce camelcase rule
loicbourgois Jan 30, 2018
c3c0bea
Remove gulp-html-validator and refactor gulpfile.js
loicbourgois Jan 30, 2018
066b3ae
Correct typo
loicbourgois Jan 30, 2018
31d6571
Move htmllint config to .json file
loicbourgois Jan 30, 2018
0940fdd
SteppedLines
loicbourgois Jan 30, 2018
8378328
Revert "Move htmllint config to .json file"
loicbourgois Jan 30, 2018
adbfb4d
Introduce error
loicbourgois Jan 30, 2018
27fa1a7
Revert "Revert "Move htmllint config to .json file""
loicbourgois Jan 30, 2018
6f96837
Fix src for lintHtmlTask()
loicbourgois Jan 30, 2018
9d2d092
Revert "Introduce error"
loicbourgois Jan 30, 2018
8ac4864
Use default config file for htmllint
loicbourgois Jan 31, 2018
ccd9cab
Fail on error
loicbourgois Jan 31, 2018
1fc07d2
Remove error
loicbourgois Feb 1, 2018
8e4c315
Remove unecessary comment
loicbourgois Feb 1, 2018
4675e85
Add link to issue for add data bug
loicbourgois Feb 1, 2018
9b59b55
Clean gulpfile
loicbourgois Feb 1, 2018
9bb89f4
Revert to inline event handler
loicbourgois Feb 1, 2018
3746bad
Disable no-unused-vars for inline events
loicbourgois Feb 1, 2018
261085f
Revert "Disable no-unused-vars for inline events"
loicbourgois Feb 1, 2018
1474880
Indentation
loicbourgois Feb 1, 2018
57fc01c
Remove old htmlTask
loicbourgois Feb 1, 2018
4a2ff8c
Fix package.json
loicbourgois Feb 1, 2018
03321b0
Fix plugin id
loicbourgois Feb 1, 2018
58db09d
eslint-disable-next-line no-unused-vars
loicbourgois Feb 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions .eslintrc.yml
View file
Open in desktop
Expand Up @@ -3,3 +3,5 @@ extends: chartjs
env:
browser: true
node: true

plugins: ['html']
13 changes: 13 additions & 0 deletions .htmllintrc.js
View file
Open in desktop
@@ -0,0 +1,13 @@
module.exports = {
Copy link
Member

Choose a reason for hiding this comment

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

Does htmllint support YAML config? It's usually more readable and easy to manipulate but also consistent with our other config files.

'rules': {
//'indent-style': 'tabs',
//'indent-width': 1,
'indent-style': false,
'indent-width': 2,
'attr-quote-style': 'double',
'spec-char-escape': false,
'attr-bans': ['align', 'background', 'bgcolor', 'border', 'frameborder', 'longdesc', 'marginwidth', 'marginheight', 'scrolling', 'syle'/*, 'width'*/],
Copy link
Member

Choose a reason for hiding this comment

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

I think there is a typo to syle but do we really need these explicit bans?

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 took the default option and commented the attributes triggering errors.
See https://github.com/htmllint/htmllint/wiki/Options#attr-bans

Copy link
Member

Choose a reason for hiding this comment

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

I see, then you can remove the 2 last values ('syle' which was initially 'style' and 'width')

'tag-bans': [/*'style', */'b', 'i'],
'id-class-style': false,
}
}
20 changes: 19 additions & 1 deletion gulpfile.js
View file
Open in desktop
Expand Up @@ -20,17 +20,26 @@ var collapse = require('bundle-collapser/plugin');
var yargs = require('yargs');
var path = require('path');
var fs = require('fs');
var htmllint = require('gulp-htmllint');
var package = require('./package.json');
var htmllintOptions = require('./.htmllintrc.js');

var argv = yargs
.option('force-output', {default: false})
.option('lint-script-elements', {default: false})
Copy link
Member

Choose a reason for hiding this comment

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

If we decide to lint our samples, then it should be part of the regular linting process. That why I would not add this --lint-script-element argument and keep script simpler by always parsing those HTML files.

.option('silent-errors', {default: false})
.option('verbose', {default: false})
.argv

var srcDir = './src/';
var outDir = './dist/';

var htmlFiles = [
Copy link
Member

Choose a reason for hiding this comment

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

This is equivalent to './samples/**/*.html'. I would put that one line directly where it's used (as we do for the rest of the gulpfile) (Note: I'm removing srcDir/outDir in the upcoming gulp rewrite).

'./samples/*.html',
'./samples/*/*.html',
'./samples/*/*/*.html',
];

var header = "/*!\n" +
" * Chart.js\n" +
" * http://chartjs.org/\n" +
Expand All @@ -50,8 +59,9 @@ gulp.task('build', buildTask);
gulp.task('package', packageTask);
gulp.task('watch', watchTask);
gulp.task('lint', lintTask);
gulp.task('lint-html', lintHtmlTask);
Copy link
Member

Choose a reason for hiding this comment

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

This task should be ran as a dependency of the lint task, not test

gulp.task('docs', docsTask);
gulp.task('test', ['lint', 'validHTML', 'unittest']);
Copy link
Member

Choose a reason for hiding this comment

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

We should also remove the validHtml associated function

gulp.task('test', ['lint', 'lint-html', 'validHTML', 'unittest']);
Copy link
Member

Choose a reason for hiding this comment

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

I think htmllint do the same job as gulp-html-validator (unmaintained?), so we should remove the 'validHTML' task and associated node dependencies in package.json.

gulp.task('size', ['library-size', 'module-sizes']);
gulp.task('server', serverTask);
gulp.task('validHTML', validHTMLTask);
Expand Down Expand Up @@ -159,6 +169,9 @@ function lintTask() {
'src/**/*.js',
'test/**/*.js'
];
if (argv.lintScriptElements) {
files = files.concat(htmlFiles);
Copy link
Member

Choose a reason for hiding this comment

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

Per my previous comment:

   var files = [
     'samples/**/*.html',
     'samples/**/*.js',
     'src/**/*.js',
     'test/**/*.js'
   ];

}

// NOTE(SB) codeclimate has 'complexity' and 'max-statements' eslint rules way too strict
// compare to what the current codebase can support, and since it's not straightforward
Expand All @@ -176,6 +189,11 @@ function lintTask() {
.pipe(eslint.failAfterError());
}

function lintHtmlTask() {
return gulp.src(htmlFiles)
.pipe(htmllint(htmllintOptions));
Copy link
Member

Choose a reason for hiding this comment

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

No need to require htmllintOptions, I think this should work:

  .pipe(htmllint({
    config: '.htmllint.js' // or .yml if supported
  }));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the source, gulp-htmllint only supports proper JSON
https://github.com/yvanavermaet/gulp-htmllint/blob/master/src/index.js#L20

}

function docsTask(done) {
const script = require.resolve('gitbook-cli/bin/gitbook.js');
const cmd = process.execPath;
Expand Down
2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Expand Up @@ -17,13 +17,15 @@
"coveralls": "^3.0.0",
"eslint": "^4.9.0",
"eslint-config-chartjs": "^0.1.0",
"eslint-plugin-html": "^4.0.2",
"gitbook-cli": "^2.3.2",
"gulp": "3.9.x",
"gulp-concat": "~2.6.x",
"gulp-connect": "~5.0.0",
"gulp-eslint": "^4.0.0",
"gulp-file": "^0.3.0",
"gulp-html-validator": "^0.0.5",
Copy link
Member

Choose a reason for hiding this comment

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

Should be removed (see previous comment)

"gulp-htmllint": "0.0.15",
Copy link
Member

Choose a reason for hiding this comment

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

"^0.0.15"

"gulp-insert": "~0.5.0",
"gulp-replace": "^0.6.1",
"gulp-size": "~2.1.0",
Expand Down
2 changes: 1 addition & 1 deletion samples/advanced/progress-bar.html
View file
Open in desktop
Expand Up @@ -93,4 +93,4 @@
</script>
</body>

</html>
</html>
2 changes: 1 addition & 1 deletion samples/charts/doughnut.html
View file
Open in desktop
Expand Up @@ -16,7 +16,7 @@

<body>
<div id="canvas-holder" style="width:40%">
<canvas id="chart-area" />
<canvas id="chart-area"></canvas>
</div>
<button id="randomizeData">Randomize Data</button>
<button id="addDataset">Add Dataset</button>
Expand Down
2 changes: 1 addition & 1 deletion samples/charts/pie.html
View file
Open in desktop
Expand Up @@ -9,7 +9,7 @@

<body>
<div id="canvas-holder" style="width:40%">
<canvas id="chart-area" />
<canvas id="chart-area"></canvas>
</div>
<button id="randomizeData">Randomize Data</button>
<button id="addDataset">Add Dataset</button>
Expand Down
4 changes: 2 additions & 2 deletions samples/scales/time/financial.html
View file
Open in desktop
Expand Up @@ -18,7 +18,7 @@
<body>
<div style="width:1000px">
<canvas id="chart1"></canvas>
<div>
</div>
<br>
<br>
Chart Type:
Expand Down Expand Up @@ -94,7 +94,7 @@

document.getElementById('update').addEventListener('click', function() {
var type = document.getElementById('type').value;
chart.config.data.datasets[0].type = type;
chart.config.data.datasets[0].type = type;
chart.update();
});

Expand Down
2 changes: 1 addition & 1 deletion samples/tooltips/custom-line.html
View file
Open in desktop
Expand Up @@ -35,7 +35,7 @@

<body>
<div id="canvas-holder1" style="width:75%;">
<canvas id="chart"/>
<canvas id="chart"></canvas>
</div>
<script>
Chart.defaults.global.pointHitDetectionRadius = 1;
Expand Down