New issue
Advanced search Search tips

Issue 685517 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

Add an eslint presubmit to Source/core/streams

Project Member Reported by ricea@chromium.org, Jan 26 2017

Issue description

Devtools is using eslint in their presubmit:

https://cs.chromium.org/search/?q=file:third_party/WebKit/Source/devtools/PRESUBMIT.py+_CheckDevtoolsStyle

Since eslint catches bugs and not just style violations it would be good to do this for streams Javascript too.
 

Comment 1 by ricea@chromium.org, Oct 31 2017

Status: Started (was: Available)
It appears that DevTools have dropped the checked-in version of eslint in favour of doing local installs: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/scripts/install_node_deps.py

However, eslint is in use by other teams: https://cs.chromium.org/chromium/src/tools/web_dev_style/presubmit_support.py
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/20c6e5af9f784dbee661d6ddd32d3b0fc5d911fd

commit 20c6e5af9f784dbee661d6ddd32d3b0fc5d911fd
Author: Adam Rice <ricea@chromium.org>
Date: Thu Nov 02 08:22:30 2017

Add eslint presubmit to Streams API implementation

ESLint will be run at upload and commit time. It can also be run manually
using "git cl presubmit --upload -f". This CL also includes a
.eslintrc.js file with many checks that are not in the default one.

Also fix existing eslint errors:

- Slightly functional changes:

* ReadableStream's constructor now uses destructuring assignment. This
  was because eslint complained about use of the "arguments" variable,
  but it was a worthwhile cleanup anyway.

- Changes that make no functional difference:

* Remove local copy of global.undefined. ESLint complains about shadowing
  it, and because "undefined" in the top-level namespace is not writable it
  wasn't actually helping security.
* Remove unused variable initialisations, mostly of variables only used
  in asserts, but also of "reader" in ReadableStreamFulfillReadRequest
* Use const for variables that aren't modified

- Format-only changes:

* Fix much bad indentation
* Wrap to 80 lines
* Split single-line function definitions
* Bin-packing of lengthy destructuring assignments has been changed to
  one-per-line. This changed as a result of using clang-format to fix the
  rest of the format. I have kept it because it avoids needing to
  reformat when adding and removing variables.
* Other miscellaneous clang-format changes.

Bug:  685517 
Change-Id: I1b654e3ae36bd223531f41fba1fe808ee77f293a
Reviewed-on: https://chromium-review.googlesource.com/746582
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513426}
[add] https://crrev.com/20c6e5af9f784dbee661d6ddd32d3b0fc5d911fd/third_party/WebKit/Source/core/streams/.eslintrc.js
[modify] https://crrev.com/20c6e5af9f784dbee661d6ddd32d3b0fc5d911fd/third_party/WebKit/Source/core/streams/ByteLengthQueuingStrategy.js
[modify] https://crrev.com/20c6e5af9f784dbee661d6ddd32d3b0fc5d911fd/third_party/WebKit/Source/core/streams/CommonOperations.js
[modify] https://crrev.com/20c6e5af9f784dbee661d6ddd32d3b0fc5d911fd/third_party/WebKit/Source/core/streams/CommonStrings.js
[modify] https://crrev.com/20c6e5af9f784dbee661d6ddd32d3b0fc5d911fd/third_party/WebKit/Source/core/streams/CountQueuingStrategy.js
[add] https://crrev.com/20c6e5af9f784dbee661d6ddd32d3b0fc5d911fd/third_party/WebKit/Source/core/streams/PRESUBMIT.py
[modify] https://crrev.com/20c6e5af9f784dbee661d6ddd32d3b0fc5d911fd/third_party/WebKit/Source/core/streams/ReadableStream.js
[modify] https://crrev.com/20c6e5af9f784dbee661d6ddd32d3b0fc5d911fd/third_party/WebKit/Source/core/streams/SimpleQueue.js
[modify] https://crrev.com/20c6e5af9f784dbee661d6ddd32d3b0fc5d911fd/third_party/WebKit/Source/core/streams/WritableStream.js

Comment 3 by ricea@chromium.org, Nov 2 2017

Status: Fixed (was: Started)

Sign in to add a comment