Upgrade linters to ESLint with stricter code style #1111

matthewlane · 2017-04-07T17:10:19Z

Type of change

Build related changes

Description of change

Updates the quality/linting system from (JSHint && JSCS) to ESLint
Updates linting system to use a stricter code style

Other information

This PR is currently configured to use JavaScript Standard Style with semicolons. Compare this to other common linting configurations with number of linting problems found against the 0.21.0 release, and the number after running eslint --fix src:

config	# of problems	# after `--fix`
eslint:recommended	121 errors, 0 warnings	121 errors, 0 warnings
Standard (with semicolons)	2254 errors, 0 warnings	321 errors, 0 warnings
Airbnb	7214 errors, 376 warnings	1897 errors, 257 warnings

Other configs, such as Canonical mentioned in #776, or a custom rule set are possible.

Compare rule sets with https://sqren.github.io/eslint-compare/

Requesting feedback from the community and @prebid/core on which code style Prebid.js should use.

* Remove jshint and jscs * Install gulp-eslint * Replace 'quality' task with 'lint' * Format lint errors with stylish * Use JavaScript Standard Style with semicolons

mkendall07 · 2017-04-17T15:59:32Z

Hey @snapwich , @protonate thoughts on this?

protonate

This is great, thanks @matthewlane

dbemiller · 2017-04-27T16:22:01Z

Changes look great. ESLint is a really nice tool.

My only real concern is not having it in the CI process anywhere. If I check out your branch and run gulp lint, it prints out the 2254 errors like you mentioned. But... it looks like the CI builds are still passing.

If there's nothing automation preventing these errors, then I don't think people will respect them. It's unrealistic to say "please don't add any more errors," because someone won't notice the difference between 2254 and 2256.

Do you think it's worth fixing these now? Or do you want to run eslint --fix locally (to get it down to ~300) and then open another issue about it?

mkendall07 · 2017-04-27T17:38:46Z

I agree with @dbemiller on this. Can we commit the eslint --fix changes, add exceptions for the ~300 existing errors and enforce strict checking in travis (add the lint gulp task into run-tests which travis invokes). ?

… stray tab character, because it was easy.

… were added during the merge.

…ntionally has eslint errors, so that we can test the CI on Travis.

protonate · 2017-04-28T22:17:23Z

@dbemiller looks like Travis CI failed on ES lint errors. Would you update the PR to pass CI and then we can merge. Thanks!

dbemiller · 2017-05-01T13:03:28Z

@protonate Yeah, that failure is on purpose. The weird behavior seems to stem from the parallelized gulp tasks stepping on each other's toes.

I think I straightened them out... but want to re-run the Travis build 4-5 times to make sure it fails reliably. Then I'll fix them and run it 4-5 times to make sure it passes reliably.

…s expected to pass now.

dbemiller · 2017-05-01T13:40:19Z

Looks to be good now ^^. Someone might want to sanity check the gulpfile before merging, though, since I declared some task dependencies that don't necessarily make a ton of sense.

I don't think they'll hurt anything (besides performance)... but it'd be good to have another set of eyes on them.

dbemiller · 2017-05-16T18:55:52Z

hey @snapwich, do you think you'll be able to get around to this sometime soon? Nearly every PR which gets merged has style issues in it, and keeping this branch up to date is getting tedious (if I don't fix them, Travis will fail all the PRs until they are fixed) Once this gets into master, CI will take care of it for us.

All the changes to actual JS code were generated by eslint --fix. When merging from master, I used git checkout --theirs and then ran eslint --fix again. Any errors which eslint couldn't fix automatically were dealt with by adding exceptions to the .eslintrc (which can be fixed in later PRs) to keep the changeset as safe as possible.

snapwich · 2017-05-16T19:58:37Z

gulpfile.js

-});
-
-gulp.task('jscs', function () {
+gulp.task('lint', () => {
  return gulp.src('src/**/*.js')


Do we want this to lint the test folder as well?

Agreed & done.

snapwich · 2017-05-16T20:01:50Z

package.json

    "faker": "^3.1.0",
    "fs.extra": "^1.3.2",
    "gulp": "^3.8.7",
    "gulp-babel": "^6.1.2",
-    "gulp-clean": "^0.3.1",
+    "gulp-clean": "^0.3.2",


If we're updating dependencies, the yarn.lock file should probably be updated as well.

good catch ^^. Done.

…keep the newly required exceptions to as small of a scope as possible.

* Upgraded to ESLint with stricter code style, for both sources and tests * Updated some dependencies and the yarn.lock file

* Added ad unit size to bid request * Fixed lint errors * Added ad unit size to bid request * Prebid 0.23.1 Release * Add trafficSourceCode + test (#1184) * pre release version bump * inclusion of popular Nordic ad sizes to default size list (#1168) inclusion of popularNordic ad sizes to default size list Removed redundant 104 size and added size 32 instead Fixed trailing comma * Add PubWise Analytics (#1151) * PubWise Analytics * Updates based on Feedback * add new rp_secure param to rubicon adapter (#1190) * Add type conversion into PrebidServer to handle inconsistent types. (#1195) * Add type conversion into prebxdserver to handle inconsistent types. * Only check for params that exist. * adding test * fix string conversion and add unit tests * fix for code review * add debug output * remove accidental commit * newline * Add dynamic bidfloor parameter to Smart Adserver Adapter (#1194) * Smart AdServer adapter Add Smart AdServer adapter with tests * fix not supported method Replace startsWith which is not supported in all browser version by lastIndexOf. * Issue with optional parameter Fix issue when no targeting is specified and remove "undefined" value passed in url * Add dynamic bidfloor option in the SmartAdServer prebid call. * Bug fix: bids served by secure creatives does not get pushed into _winningBids (#1192) * Upgrade linters to ESLint with stricter code style (#1111) * Upgraded to ESLint with stricter code style, for both sources and tests * Updated some dependencies and the yarn.lock file * Add Support for DigiTrust in Rubicon Adapter (#1201) * Add support for DigiTrust * Add rubicon tests covering digitrust failures * Remove whitespace in Rubicon adapter * HIRO Media: Remove batching mechanism and use AJAX instead of JSONP (#1133) * Remove batching mechanism and use AJAX instead of JSONP * Fix `undefined` value checks * Rename secureCreatives file and lint (#1203) * Rename secureCreatives file and lint * Updated package script for linting * Use 'gulp run-tests' in package script for testing * updated tag (#1212) * Common user-sync (#1144) * Changed “bidRequest” to “bid” for clarity

…built * 'master' of https://github.com/prebid/Prebid.js: (23 commits) Increment pre version Probed 0.24.0 Release Beachfront adapter - add ad unit size (prebid#1183) Thoughtleadr adapter - fix postMessage (prebid#1207) When prebid server issues a no-bid response, call addBidResponse for every adUnit requested (prebid#1204) Improvement/timeout xhr (prebid#1172) Add native support (prebid#1072) Improvement/alias queue (prebid#1156) Updated documentaion (prebid#1160) Improvement/prebid iframes amp pages (prebid#1119) Fixes prebid#1114 possible xss issue (prebid#1186) Allowed setTargetingForGPTAsync() to target specific ad unit codes. (prebid#1158) updated tag (prebid#1212) Common user-sync (prebid#1144) Rename secureCreatives file and lint (prebid#1203) HIRO Media: Remove batching mechanism and use AJAX instead of JSONP (prebid#1133) Add Support for DigiTrust in Rubicon Adapter (prebid#1201) Upgrade linters to ESLint with stricter code style (prebid#1111) Add dynamic bidfloor parameter to Smart Adserver Adapter (prebid#1194) Bug fix: bids served by secure creatives does not get pushed into _winningBids (prebid#1192) ...

* Added ad unit size to bid request * Fixed lint errors * Added ad unit size to bid request * Prebid 0.23.1 Release * Add trafficSourceCode + test (prebid#1184) * pre release version bump * inclusion of popular Nordic ad sizes to default size list (prebid#1168) inclusion of popularNordic ad sizes to default size list Removed redundant 104 size and added size 32 instead Fixed trailing comma * Add PubWise Analytics (prebid#1151) * PubWise Analytics * Updates based on Feedback * add new rp_secure param to rubicon adapter (prebid#1190) * Add type conversion into PrebidServer to handle inconsistent types. (prebid#1195) * Add type conversion into prebxdserver to handle inconsistent types. * Only check for params that exist. * adding test * fix string conversion and add unit tests * fix for code review * add debug output * remove accidental commit * newline * Add dynamic bidfloor parameter to Smart Adserver Adapter (prebid#1194) * Smart AdServer adapter Add Smart AdServer adapter with tests * fix not supported method Replace startsWith which is not supported in all browser version by lastIndexOf. * Issue with optional parameter Fix issue when no targeting is specified and remove "undefined" value passed in url * Add dynamic bidfloor option in the SmartAdServer prebid call. * Bug fix: bids served by secure creatives does not get pushed into _winningBids (prebid#1192) * Upgrade linters to ESLint with stricter code style (prebid#1111) * Upgraded to ESLint with stricter code style, for both sources and tests * Updated some dependencies and the yarn.lock file * Add Support for DigiTrust in Rubicon Adapter (prebid#1201) * Add support for DigiTrust * Add rubicon tests covering digitrust failures * Remove whitespace in Rubicon adapter * HIRO Media: Remove batching mechanism and use AJAX instead of JSONP (prebid#1133) * Remove batching mechanism and use AJAX instead of JSONP * Fix `undefined` value checks * Rename secureCreatives file and lint (prebid#1203) * Rename secureCreatives file and lint * Updated package script for linting * Use 'gulp run-tests' in package script for testing * updated tag (prebid#1212) * Common user-sync (prebid#1144) * Changed “bidRequest” to “bid” for clarity

* Upgraded to ESLint with stricter code style, for both sources and tests * Updated some dependencies and the yarn.lock file

* Added ad unit size to bid request * Fixed lint errors * Added ad unit size to bid request * Prebid 0.23.1 Release * Add trafficSourceCode + test (prebid#1184) * pre release version bump * inclusion of popular Nordic ad sizes to default size list (prebid#1168) inclusion of popularNordic ad sizes to default size list Removed redundant 104 size and added size 32 instead Fixed trailing comma * Add PubWise Analytics (prebid#1151) * PubWise Analytics * Updates based on Feedback * add new rp_secure param to rubicon adapter (prebid#1190) * Add type conversion into PrebidServer to handle inconsistent types. (prebid#1195) * Add type conversion into prebxdserver to handle inconsistent types. * Only check for params that exist. * adding test * fix string conversion and add unit tests * fix for code review * add debug output * remove accidental commit * newline * Add dynamic bidfloor parameter to Smart Adserver Adapter (prebid#1194) * Smart AdServer adapter Add Smart AdServer adapter with tests * fix not supported method Replace startsWith which is not supported in all browser version by lastIndexOf. * Issue with optional parameter Fix issue when no targeting is specified and remove "undefined" value passed in url * Add dynamic bidfloor option in the SmartAdServer prebid call. * Bug fix: bids served by secure creatives does not get pushed into _winningBids (prebid#1192) * Upgrade linters to ESLint with stricter code style (prebid#1111) * Upgraded to ESLint with stricter code style, for both sources and tests * Updated some dependencies and the yarn.lock file * Add Support for DigiTrust in Rubicon Adapter (prebid#1201) * Add support for DigiTrust * Add rubicon tests covering digitrust failures * Remove whitespace in Rubicon adapter * HIRO Media: Remove batching mechanism and use AJAX instead of JSONP (prebid#1133) * Remove batching mechanism and use AJAX instead of JSONP * Fix `undefined` value checks * Rename secureCreatives file and lint (prebid#1203) * Rename secureCreatives file and lint * Updated package script for linting * Use 'gulp run-tests' in package script for testing * updated tag (prebid#1212) * Common user-sync (prebid#1144) * Changed “bidRequest” to “bid” for clarity

Upgrade linters to ESLint with stricter code style

ebdf056

* Remove jshint and jscs * Install gulp-eslint * Replace 'quality' task with 'lint' * Format lint errors with stylish * Use JavaScript Standard Style with semicolons

matthewlane added the in progress label Apr 7, 2017

protonate approved these changes Apr 17, 2017

View reviewed changes

protonate requested a review from dbemiller April 26, 2017 18:18

protonate added the needs review label Apr 26, 2017

dbemiller mentioned this pull request Apr 27, 2017

Feature/editorconfig #1147

Merged

1 task

protonate assigned matthewlane Apr 28, 2017

protonate added needs update and removed needs review labels Apr 28, 2017

dbemiller added 5 commits April 28, 2017 14:10

Ran eslint --fix to automatically fix errors.

1a6d051

Added exceptions for rules which Prebid already violates. Removed one…

bf59f5d

… stray tab character, because it was easy.

Merged from main. All conflicts resolved using git checkout --theirs.

acf69e0

Ran eslint --fix again. Added more exceptions for new violations that…

377273f

… were added during the merge.

Hacked my way around the gulp eslint + mocha errors. This commit inte…

dcf82ee

…ntionally has eslint errors, so that we can test the CI on Travis.

protonate assigned dbemiller and unassigned matthewlane Apr 28, 2017

Uncommented the last style check, and alphabetized the list. Linter i…

b0232a7

…s expected to pass now.

dbemiller approved these changes May 1, 2017

View reviewed changes

dbemiller removed the needs update label May 1, 2017

dbemiller mentioned this pull request May 1, 2017

Adding Video Support to Conversant Adapter #1153

Merged

Merged from master. Fixed more conflicts.

eec545b

mkendall07 requested a review from snapwich May 10, 2017 13:54

dbemiller added 2 commits May 11, 2017 12:42

Merged from master. Fixed more conflicts.

5cf3257

Merge branch 'master' of https://github.com/prebid/Prebid.js into eslint

b7cfe31

Fixed style issues.

b94f88b

snapwich reviewed May 16, 2017

View reviewed changes

dbemiller added 2 commits May 16, 2017 16:29

Extended linting to cover test folder. Added an override eslintrc to …

55155bc

…keep the newly required exceptions to as small of a scope as possible.

Updated the yarn.lock file.

92d70af

dbemiller force-pushed the eslint branch from 7e76dc7 to 92d70af Compare May 16, 2017 20:53

Merged from master. Fixed more style errors.

0edfb26

snapwich approved these changes May 16, 2017

View reviewed changes

Merged from master. Fixed conflicts and style errors.

4d3fef5

dbemiller merged commit 0799a64 into master May 17, 2017

mkendall07 removed the in progress label May 17, 2017

dbemiller deleted the eslint branch May 17, 2017 01:32

jaiminpanchal27 mentioned this pull request May 17, 2017

Prevent 'about:' protocol in _publisherTagUrl #1170

Merged

1 task

bfmdeploy pushed a commit to bfmdeploy/Prebid.js that referenced this pull request May 19, 2017

Upgrade linters to ESLint with stricter code style (prebid#1111)

e8be4ac

* Upgraded to ESLint with stricter code style, for both sources and tests * Updated some dependencies and the yarn.lock file

dbemiller mentioned this pull request May 23, 2017

E-Planning adapter #1169

Closed

9 tasks

This was referenced Jun 7, 2017

Fixing a bug with gulp watch rebuilding #1276

Merged

Modules #1177

Merged

matthewlane mentioned this pull request Jun 14, 2017

Integrate consistent code formatting #1073

Closed

dbemiller mentioned this pull request Jun 21, 2017

Removed all jshint comments #1315

Merged

dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018

Upgrade linters to ESLint with stricter code style (prebid#1111)

5daec55

* Upgraded to ESLint with stricter code style, for both sources and tests * Updated some dependencies and the yarn.lock file

jsnellbaker mentioned this pull request Dec 20, 2019

update/fix cases of undefined variables #4661

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade linters to ESLint with stricter code style #1111

Upgrade linters to ESLint with stricter code style #1111

matthewlane commented Apr 7, 2017 •

edited

Loading

mkendall07 commented Apr 17, 2017

protonate left a comment

dbemiller commented Apr 27, 2017

mkendall07 commented Apr 27, 2017

protonate commented Apr 28, 2017

dbemiller commented May 1, 2017

dbemiller commented May 1, 2017

dbemiller commented May 16, 2017

snapwich May 16, 2017

dbemiller May 16, 2017

snapwich May 16, 2017

dbemiller May 16, 2017

Upgrade linters to ESLint with stricter code style #1111

Upgrade linters to ESLint with stricter code style #1111

Conversation

matthewlane commented Apr 7, 2017 • edited Loading

Type of change

Description of change

Other information

mkendall07 commented Apr 17, 2017

protonate left a comment

Choose a reason for hiding this comment

dbemiller commented Apr 27, 2017

mkendall07 commented Apr 27, 2017

protonate commented Apr 28, 2017

dbemiller commented May 1, 2017

dbemiller commented May 1, 2017

dbemiller commented May 16, 2017

snapwich May 16, 2017

Choose a reason for hiding this comment

dbemiller May 16, 2017

Choose a reason for hiding this comment

snapwich May 16, 2017

Choose a reason for hiding this comment

dbemiller May 16, 2017

Choose a reason for hiding this comment

matthewlane commented Apr 7, 2017 •

edited

Loading