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

Run linter on build-system #11325

Merged
merged 2 commits into from Sep 19, 2017
Merged

Run linter on build-system #11325

merged 2 commits into from Sep 19, 2017

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Sep 18, 2017

This PR adds ES5 compatible linter rules for the build-system directory, and ensures that any file in that directory that is touched by a PR is also linted. For now, we do this in warning mode, until a first round of fixes is checked in.

To run this locally, use:

gulp lint --build_system

Coming up: A follow up PR with slightly more sane lint rules, and an initial round of fixes for the build-system files.

Partial fix for #11273.

@rsimha
Copy link
Contributor Author

rsimha commented Sep 18, 2017

/to @choumx @erwinmombay

@rsimha
Copy link
Contributor Author

rsimha commented Sep 19, 2017

/to @cramforce

@rsimha rsimha merged commit 10a97a8 into ampproject:master Sep 19, 2017
@rsimha rsimha deleted the 2017-09-18-BuildSystemLint branch September 19, 2017 14:57
@@ -19,9 +19,11 @@
var argv = require('minimist')(process.argv.slice(2));
var config = require('../config');
var eslint = require('gulp-eslint');
var getStdout = require('../exec.js').getStdout;
Copy link
Member

Choose a reason for hiding this comment

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

remove .js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

* @return {!Stream} Readable stream
* Initializes the linter stream based on globs
* @param {!object} globs
* @param {!object} streamOptions
Copy link
Member

Choose a reason for hiding this comment

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

!Object instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

* Runs the linter on the given stream using the given options.
* @param {!string} path
* @param {!ReadableStream} stream
* @param {!object} options
Copy link
Member

Choose a reason for hiding this comment

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

!Object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@rsimha
Copy link
Contributor Author

rsimha commented Sep 19, 2017

@erwinmombay Fixes coming in a follow up PR. If only we had a linter to catch these ;)

@erwinmombay
Copy link
Member

erwinmombay commented Sep 19, 2017

most of these wouldnt have been caught by the linter as they are bad type annotations. (we don't run the type checker on the build system either, probly not worth it but still good to have the correct annotations)

@rsimha
Copy link
Contributor Author

rsimha commented Sep 21, 2017

All your comments are addressed in the follow up PRs listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants