Closed Bug 1194314 Opened 7 years ago Closed 7 years ago

ESLint cleanup for tests

Categories

(DevTools :: Style Editor, defect)

42 Branch
defect
Not set
normal

Tracking

(firefox43 fixed)

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: jfong, Assigned: jfong)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Comment on attachment 8648115 [details] [diff] [review]
Bug1194314.patch

Review of attachment 8648115 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! r=me with the comments addressed

::: browser/devtools/styleeditor/test/browser_styleeditor_inline_friendly_names.js
@@ +42,5 @@
>      "URI is the identifier of style sheet file.");
>  
> +  is(ui.getStyleSheetIdentifier(fakeInlineStyleSheet),
> +    "inline-2-at-http://example.com/",
> +    "Inline style sheets are identified by their page and position at that " +

Rather than wrapping the line, could we rephrase to something like:

"Inline sheets are identified by their page and position in the page."

@@ +70,4 @@
>    is(firstEditor.friendlyName, SAVE_PATH,
>      "Friendly name is correct for the saved inline style sheet.");
>    isnot(secondEditor.friendlyName, SAVE_PATH,
> +    "Friendly name is for the second inline style sheet is not the same as " +

Rather than wrapping the line, could we rephrase to something like:

"Friendly name for the second inline sheet isn't the same as the first."

::: browser/devtools/styleeditor/test/browser_styleeditor_pretty.js
@@ +7,5 @@
>  // untouched.
>  
>  const TESTCASE_URI = TEST_BASE_HTTP + "minified.html";
>  
> +/*

Should this be two different comments?  One above the declaration for PRETTIFIED_SOURCE and one above ORIGINAL_SOURCE

::: browser/devtools/styleeditor/test/browser_styleeditor_reload.js
@@ +20,5 @@
>    yield ui.selectStyleSheet(ui.editors[1].styleSheet, LINE_NO, COL_NO);
>  
>    info("Reloading page.");
> +  executeInContent("devtools:test:reload", {}, {},
> +    /* no response */

Should false be on the same line as the comment here?

::: browser/devtools/styleeditor/test/browser_styleeditor_sourcemap_watching.js
@@ +137,5 @@
>      .getService(Ci.nsIScriptableInputStream);
>  
> +  let channel = Services.io.newChannel2(srcChromeURL,
> +                                        null,
> +                                        null,

Is there a rule about not having this comment on the same line as the arg, or is it that this is > 80 characters?  b/c I think the comment right after 'null' makes it more clear

@@ +143,2 @@
>                                          null,
> +                                        Services.scriptSecurityManager

if it would help, you could have a line above like 

`let principal = Services.scriptSecurityManager.getSystemPrincipal();`

then use principal in the argument list.

::: browser/devtools/styleeditor/test/browser_styleeditor_sourcemaps.js
@@ +43,5 @@
>      "",
> +    "/*# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJma" +
> +    "WxlIjoiIiwic291cmNlcyI6WyJzYXNzL2NvbnRhaW5lZC5zY3NzIl0sIm5hbWVzIjpbXSwi" +
> +    "bWFwcGluZ3MiOiJBQUVBO0VBQ0UsT0FISyIsInNvdXJjZXNDb250ZW50IjpbIiRwaW5rOiA" +
> +    "jZjA2O1xuXG4jaGVhZGVyIHtcbiAgY29sb3I6ICRwaW5rO1xufSJdfQ==*/"

I think we should have an exception to the rules for data URIs or other necessarily long strings.  What do you think?  I'm not crazy about this change, but it's fine.
Attachment #8648115 - Flags: review?(bgrinstead) → review+
(In reply to Brian Grinstead [:bgrins] from comment #2)
> Comment on attachment 8648115 [details] [diff] [review]
> Bug1194314.patch
> 
> Review of attachment 8648115 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks! r=me with the comments addressed
> 
> :::
> browser/devtools/styleeditor/test/browser_styleeditor_inline_friendly_names.
> js
> @@ +42,5 @@
> >      "URI is the identifier of style sheet file.");
> >  
> > +  is(ui.getStyleSheetIdentifier(fakeInlineStyleSheet),
> > +    "inline-2-at-http://example.com/",
> > +    "Inline style sheets are identified by their page and position at that " +
> 
> Rather than wrapping the line, could we rephrase to something like:
> 
> "Inline sheets are identified by their page and position in the page."

Yep, I'll reword.

> 
> @@ +70,4 @@
> >    is(firstEditor.friendlyName, SAVE_PATH,
> >      "Friendly name is correct for the saved inline style sheet.");
> >    isnot(secondEditor.friendlyName, SAVE_PATH,
> > +    "Friendly name is for the second inline style sheet is not the same as " +
> 
> Rather than wrapping the line, could we rephrase to something like:
> 
> "Friendly name for the second inline sheet isn't the same as the first."
> 

Yep, I'll reword.

> ::: browser/devtools/styleeditor/test/browser_styleeditor_pretty.js
> @@ +7,5 @@
> >  // untouched.
> >  
> >  const TESTCASE_URI = TEST_BASE_HTTP + "minified.html";
> >  
> > +/*
> 
> Should this be two different comments?  One above the declaration for
> PRETTIFIED_SOURCE and one above ORIGINAL_SOURCE

Yes, you are correct.

> 
> ::: browser/devtools/styleeditor/test/browser_styleeditor_reload.js
> @@ +20,5 @@
> >    yield ui.selectStyleSheet(ui.editors[1].styleSheet, LINE_NO, COL_NO);
> >  
> >    info("Reloading page.");
> > +  executeInContent("devtools:test:reload", {}, {},
> > +    /* no response */
> 
> Should false be on the same line as the comment here?
> 
> :::
> browser/devtools/styleeditor/test/browser_styleeditor_sourcemap_watching.js
> @@ +137,5 @@
> >      .getService(Ci.nsIScriptableInputStream);
> >  
> > +  let channel = Services.io.newChannel2(srcChromeURL,
> > +                                        null,
> > +                                        null,
> 
> Is there a rule about not having this comment on the same line as the arg,
> or is it that this is > 80 characters?  b/c I think the comment right after
> 'null' makes it more clear
> 

Yep, there is an ESLint rule about not having comments on the same line as code - as discussed on IRC, I'll just remove the comment.

> @@ +143,2 @@
> >                                          null,
> > +                                        Services.scriptSecurityManager
> 
> if it would help, you could have a line above like 
> 
> `let principal = Services.scriptSecurityManager.getSystemPrincipal();`
> 
> then use principal in the argument list.
> 
> ::: browser/devtools/styleeditor/test/browser_styleeditor_sourcemaps.js
> @@ +43,5 @@
> >      "",
> > +    "/*# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJma" +
> > +    "WxlIjoiIiwic291cmNlcyI6WyJzYXNzL2NvbnRhaW5lZC5zY3NzIl0sIm5hbWVzIjpbXSwi" +
> > +    "bWFwcGluZ3MiOiJBQUVBO0VBQ0UsT0FISyIsInNvdXJjZXNDb250ZW50IjpbIiRwaW5rOiA" +
> > +    "jZjA2O1xuXG4jaGVhZGVyIHtcbiAgY29sb3I6ICRwaW5rO1xufSJdfQ==*/"
> 
> I think we should have an exception to the rules for data URIs or other
> necessarily long strings.  What do you think?  I'm not crazy about this
> change, but it's fine.

Yeah I thought about it, but I decided to be consistent w/ the rules - I'm okay either way but it does create a warning about lines exceeding some point n (in this case, 80). I could revert but it just means it makes it more confusing for contributors if/when they have to either a) follow the ESLint rules exactly or b) have exceptions which we internally allow/deny. If we do agree with b) then we should outline this somewhere in our docs or we should adjust the .eslintrc file to not complain about it. Thoughts?
Flags: needinfo?(bgrinstead)
(In reply to Jen Fong-Adwent [:ednapiranha] from comment #3)
> > ::: browser/devtools/styleeditor/test/browser_styleeditor_sourcemaps.js
> > @@ +43,5 @@
> > >      "",
> > > +    "/*# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJma" +
> > > +    "WxlIjoiIiwic291cmNlcyI6WyJzYXNzL2NvbnRhaW5lZC5zY3NzIl0sIm5hbWVzIjpbXSwi" +
> > > +    "bWFwcGluZ3MiOiJBQUVBO0VBQ0UsT0FISyIsInNvdXJjZXNDb250ZW50IjpbIiRwaW5rOiA" +
> > > +    "jZjA2O1xuXG4jaGVhZGVyIHtcbiAgY29sb3I6ICRwaW5rO1xufSJdfQ==*/"
> > 
> > I think we should have an exception to the rules for data URIs or other
> > necessarily long strings.  What do you think?  I'm not crazy about this
> > change, but it's fine.
> 
> Yeah I thought about it, but I decided to be consistent w/ the rules - I'm
> okay either way but it does create a warning about lines exceeding some
> point n (in this case, 80). I could revert but it just means it makes it
> more confusing for contributors if/when they have to either a) follow the
> ESLint rules exactly or b) have exceptions which we internally allow/deny.
> If we do agree with b) then we should outline this somewhere in our docs or
> we should adjust the .eslintrc file to not complain about it. Thoughts?

Let's forward this to Patrick.  My suggestion is we add a new rule that looks for something like \"(.*)data:(.+)/(.+);(.+)\" in the line and then allows it to exceed 80 characters.  Note: I don't know how possible this would be, but it'd be good if we get his opinion about whether that would even be desirable.

In the meantime, let's land it how you have it (so it's passing the linter as-is).
Flags: needinfo?(bgrinstead) → needinfo?(pbrosset)
Attached patch Bug1194314.patchSplinter Review
Attachment #8648115 - Attachment is obsolete: true
Attachment #8648173 - Flags: review+
(In reply to Brian Grinstead [:bgrins] from comment #4)
> (In reply to Jen Fong-Adwent [:ednapiranha] from comment #3)
> > > ::: browser/devtools/styleeditor/test/browser_styleeditor_sourcemaps.js
> > > @@ +43,5 @@
> > > >      "",
> > > > +    "/*# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJma" +
> > > > +    "WxlIjoiIiwic291cmNlcyI6WyJzYXNzL2NvbnRhaW5lZC5zY3NzIl0sIm5hbWVzIjpbXSwi" +
> > > > +    "bWFwcGluZ3MiOiJBQUVBO0VBQ0UsT0FISyIsInNvdXJjZXNDb250ZW50IjpbIiRwaW5rOiA" +
> > > > +    "jZjA2O1xuXG4jaGVhZGVyIHtcbiAgY29sb3I6ICRwaW5rO1xufSJdfQ==*/"
> > > 
> > > I think we should have an exception to the rules for data URIs or other
> > > necessarily long strings.  What do you think?  I'm not crazy about this
> > > change, but it's fine.
> > 
> > Yeah I thought about it, but I decided to be consistent w/ the rules - I'm
> > okay either way but it does create a warning about lines exceeding some
> > point n (in this case, 80). I could revert but it just means it makes it
> > more confusing for contributors if/when they have to either a) follow the
> > ESLint rules exactly or b) have exceptions which we internally allow/deny.
> > If we do agree with b) then we should outline this somewhere in our docs or
> > we should adjust the .eslintrc file to not complain about it. Thoughts?
> 
> Let's forward this to Patrick.  My suggestion is we add a new rule that
> looks for something like \"(.*)data:(.+)/(.+);(.+)\" in the line and then
> allows it to exceed 80 characters.  Note: I don't know how possible this
> would be, but it'd be good if we get his opinion about whether that would
> even be desirable.
Since we added the .eslintrc files, we started noticing things like this. Rules that work for us 99% of the times, but there's this 1% cases where we'd like the rule to just not be there. As you said, either we live with this and write something in the doc (that no one will ever read), or we make another rule.
ESLint was chosen also because it's very easily extensible, so I'd say, let's use this capability and start creating our own rules.
However, it turns out the max-len rule already support a parameter called ignorePattern which we could use to skip over data-uris: http://eslint.org/docs/rules/max-len#options
Flags: needinfo?(pbrosset)
Keywords: checkin-needed
See Also: → 1195354
https://hg.mozilla.org/mozilla-central/rev/9e51cc2ce944
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.