Skip to content

Enable travis linting. #333

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
Merged
merged 8 commits into from
Mar 6, 2018
Merged

Enable travis linting. #333

merged 8 commits into from
Mar 6, 2018

Conversation

samtstern
Copy link
Contributor

@samtstern samtstern commented Mar 2, 2018 •
edited
Loading

samtstern
samtstern commented Mar 2, 2018
View reviewed changes
assistant-say-number/functions/package.json
},
"scripts": {
"lint": "./node_modules/.bin/eslint .",
"lint": "./node_modules/.bin/eslint --max-warnings=0 .",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The --max-warnings=0 is basically a strict mode to fail on warnings.
Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good to me. @inlined FYI.
samtstern
samtstern commented Mar 2, 2018
View reviewed changes
quickstarts/time-server/functions/index.js
// [START usingMiddleware]
// Enable CORS using the `cors` express middleware.
cors(req, res, () => {
return cors(req, res, () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the one lint violation I found!
samtstern
samtstern commented Mar 2, 2018
View reviewed changes
es2017-transpile/functions/index.js Outdated
@@ -0,0 +1,27 @@
/**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any idea why this is here? Should it be gitignored?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that's the built/transpiled version of index.es7 it should not be checked in (it can be ignored).
@samtstern samtstern changed the title Enable travis linting and testing. Enable travis linting. Mar 2, 2018
@samtstern
Copy link
Contributor Author

@nicolasgarnier note that I was not able to get lerna bootstrap to run on Travis in a reasonable about of time (I think because it spins up ~50 node processes) so I couldn't run the test.

If we can find a more efficient way to npm install in each module then we could run lerna run test to hit all of the tests.

We're able to do lerna run lint because that actually doesn't require installing anything besides the linter.
@samtstern samtstern requested a review from nicolasgarnier March 2, 2018 20:50
nicolasgarnier
nicolasgarnier reviewed Mar 2, 2018
View reviewed changes
Copy link
Contributor

@nicolasgarnier nicolasgarnier left a comment

Choose a reason for hiding this comment

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

Looks good thanks!

Actually we had only added ESlint files for the Node/Functions files, some projects have client-side JS as well. Might be worth adding [an] ESlint file[s] for these as well wdyt?
.travis.yml Outdated
- npm install -g eslint
- lerna bootstrap
script:
- ./scripts/test.sh No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

Add end of file line break
es2017-transpile/functions/index.js Outdated
@@ -0,0 +1,27 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that's the built/transpiled version of index.es7 it should not be checked in (it can be ignored).
lerna.json Outdated
"username-password-auth/functions"
],
"version": "1.0.0"
} No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

Add end of file line break.
assistant-say-number/functions/package.json
},
"scripts": {
"lint": "./node_modules/.bin/eslint .",
"lint": "./node_modules/.bin/eslint --max-warnings=0 .",
Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good to me. @inlined FYI.
.travis.yml Outdated
- npm install -g typescript
- npm install -g tslint
script:
- ./scripts/test.sh No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

Add end of file line break
paypal/.gitignore Outdated
firebase-debug.log
functions/eslintrc.js
.*
functions/eslintrc.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Add end of file line break ;P
@nicolasgarnier
Copy link
Contributor

For lerna run test would it be possible to restrict it to only the modules that actually have tests? right now there is only one (quickstart > database) so we don't need to npm install on all the samples.
@samtstern
Copy link
Contributor Author

Ok these are all the other directories that contain .js and are not functions:

$ find . -type f -name "*.js" -not -path "*functions*" | xargs dirname | sort | uniq ./authenticated-json-api/public ./authorized-https-endpoint/public ./email-confirmation/public ./exif-images/public ./fcm-notifications/public ./fulltext-search-firestore/public ./instagram-auth/public ./isomorphic-react-app/src ./isomorphic-react-app/src/containers ./linkedin-auth/public ./nextjs-with-firebase-hosting/src/app ./nextjs-with-firebase-hosting/src/app/components ./nextjs-with-firebase-hosting/src/app/pages ./presence-firestore/public ./quickstarts/email-users/public ./spotify-auth/public ./text-moderation/public ./username-password-auth/public

I think it should be a separate issue to add an .eslintrc to each of those since I don't really feel qualified to make the style decisions there.
samtstern
samtstern commented Mar 2, 2018
View reviewed changes
quickstarts/uppercase/functions/package.json Outdated
"scripts": {
"test": "./node_modules/.bin/mocha --reporter spec",
"lint": "./node_modules/.bin/eslint .",
"build": "npm install",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolasgarnier got the tests running by adding this build step.
Copy link
Member

Choose a reason for hiding this comment

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

build should not do NPM install; that takes way too long and is needed so infrequently. It should be part of our travis rules.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I want is for "test" do depend on "install" so that when I do lerna run test it will run npm install in only the directories that have tests.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about I make a travis-test target that does npm install && npm run test
@nicolasgarnier nicolasgarnier merged commit 1c3639b into master Mar 6, 2018
@nicolasgarnier
Copy link
Contributor

LGTM Thanks @samtstern !

We'll add the lin files for the frontend JS files separately as you suggested.
@nicolasgarnier nicolasgarnier deleted the travis-ci branch March 6, 2018 00:09
@samtstern samtstern mentioned this pull request Mar 6, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants

@samtstern @nicolasgarnier @inlined