Fix a few Closure and JSDoc issues. #1441
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ebidel
suggested changes
Jan 8, 2017
| @@ -255,6 +255,7 @@ class Driver { | |||
| * If our main document URL redirects, we will update options.url accordingly | |||
| * As such, options.url will always represent the post-redirected URL. | |||
| * options.initialUrl is the pre-redirect URL that things started with | |||
| * @param {!string} opts | |||
| @@ -38,6 +38,8 @@ class Emitter extends EventEmitter { | |||
| /** | |||
| * Fires off all status updates. Listen with | |||
| * `require('lib/log').events.addListener('status', callback)` | |||
| * @param {string} title | |||
|
|
|||
There was a problem hiding this comment.
remove extras newline and add args param
There was a problem hiding this comment.
What type is args? Object?
| @@ -342,7 +342,7 @@ class LighthouseViewerReport { | |||
|
|
|||
| /** | |||
| * Downloads a file (blob) using a[download]. | |||
| * @param {Blob|File} The file to save. | |||
| * @param {string} blob The file to save. | |||
|
Yup, I'll address your comments later.
…On Jan 8, 2017 20:22, "Eric Bidelman" ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In lighthouse-core/gather/driver.js
<#1441 (review)>
:
> @@ -255,6 +255,7 @@ class Driver {
* If our main document URL redirects, we will update options.url accordingly
* As such, options.url will always represent the post-redirected URL.
* options.initialUrl is the pre-redirect URL that things started with
+ * @param {!string} opts
!Object
------------------------------
In lighthouse-core/lib/log.js
<#1441 (review)>
:
> @@ -38,6 +38,8 @@ class Emitter extends EventEmitter {
/**
* Fires off all status updates. Listen with
* `require('lib/log').events.addListener('status', callback)`
+ * @param {string} title
+
remove extras newline and add args param
------------------------------
In lighthouse-viewer/app/src/lighthouse-report-viewer.js
<#1441 (review)>
:
> @@ -342,7 +342,7 @@ class LighthouseViewerReport {
/**
* Downloads a file (blob) using a[download].
- * @param {Blob|File} The file to save.
+ * @param {string} blob The file to save.
this should stay Blob|File
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1441 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAVVteTDEe6zrOu3QScQf53m2KnASuDXks5rQSmCgaJpZM4Ldr-0>
.
|
XhmikosR
changed the title
ebidel
suggested changes
Jan 9, 2017
| @@ -38,6 +38,8 @@ class Emitter extends EventEmitter { | |||
| /** | |||
| * Fires off all status updates. Listen with | |||
| * `require('lib/log').events.addListener('status', callback)` | |||
| * @param {string} title | |||
| * @param {function(...)} args | |||
There was a problem hiding this comment.
From callsite usage, it look like it's an optional arguments array.
There was a problem hiding this comment.
| @@ -48,6 +50,7 @@ class Emitter extends EventEmitter { | |||
| /** | |||
| * Fires off all warnings. Listen with | |||
| * `require('lib/log').events.addListener('warning', callback)` | |||
| * @param {function(...)} args | |||
There was a problem hiding this comment.
non-optional arguments array
| @@ -121,6 +121,7 @@ document.addEventListener('DOMContentLoaded', _ => { | |||
| /** | |||
| * Generates a document fragment containing a list of checkboxes and labels | |||
| * for the aggregation categories. | |||
| * @param {!Object<null>} list | |||
There was a problem hiding this comment.
@brendankenny is Object<null> a thing? I only found https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the-Closure-Compiler#type-application
| @@ -267,6 +268,7 @@ class LighthouseViewerReport { | |||
|
|
|||
| /** | |||
| * Enables pasting a JSON report on the page. | |||
| * @param {string} e Event name. | |||
There was a problem hiding this comment.
you can remove "Event name". It's just an event and obvious. Remove throughout.
|
@ebidel: I reverted the changes. I don't think it's worth me spending more time on this especially since closure doesn't run in CI. So feel free to push here for further improvements, or close it, it's your call. I was just trying to solve the easy Closure issues. |
|
Understood. Appreciate all your help so far. We can update the rest. |
ebidel
approved these changes
Jan 9, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.