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

Add JSHint and JSCS linting to the build process #286

Merged
merged 3 commits into from
Dec 11, 2015
Merged

Conversation

tehapo
Copy link

@tehapo tehapo commented Dec 10, 2015

A lot of lines were touched in this pull request. 💦 Still there are no actual code logic changes, except a repeated test data creation pattern was simplified to avoid warnings about creating functions inside a loop.

I self-reviewed all the changes after automated find and replaces and verified manually that all demos still work.

PS. I'll just leave this Kekkonen reference here also. 😆

Review on Reviewable

@manolo
Copy link
Member

manolo commented Dec 10, 2015

A lot of chantes here, I have not gone over them, so, can you explain what are the things we should focus to review?
Also how did you formatted all those files? are there new gulp tasks ?

@tehapo
Copy link
Author

tehapo commented Dec 11, 2015

(By the way, if you feel like this is too big of a change, we can abandon this with no hard feelings from me and revisit the linting later after smoke has cleared from the 1.0 release.)

The vast majority of changes are simply changing quotation marks (") to single quotes (') as required by the google preset configured in JSCS. Also adding missing spaces and semicolons was a big part of the change.

I think the only focus for this review should be that there are no accidental code changes, except the test data generation in some examples, which I already mentioned. Configuration of the linters is basically just copied from vaadin/vaadin-combo-box with the addition of allowed (non-standard) JSDoc annotations required by the Grid docs (see .jscsrc file).

The formatting changes were done manually (and with find and replace feature) after first running the new gulp lint:js and gulp lint:html tasks to find out what lines are still conflicting with the style guides.

@manolo
Copy link
Member

manolo commented Dec 11, 2015

Reviewed 35 of 40 files at r1, 4 of 5 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

manolo added a commit that referenced this pull request Dec 11, 2015
Add JSHint and JSCS linting to the build process
@manolo manolo merged commit 2691df7 into master Dec 11, 2015
@manolo manolo removed the in review label Dec 11, 2015
@manolo manolo deleted the build/linting branch December 11, 2015 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants