1184746 - Eslint cleanup for rule-view & computed-view

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Julian Descottes]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.132 Safari/537.36

Steps to reproduce:

Follow-up to Bug 1164343.

Review styleinspector/rule-view.js and styleinspector/computed-view.js for eslint warnings and violations.

Comment 1

[Julian Descottes]

Attachment: Bug1184746.computedview.part1.v1.patch

Only minor nits in this one : curly braces after line return, missing spaces ...

Comment 2

[Julian Descottes]

Attachment: Bug1184746.computedview.part2.v1.patch

Adding missing globals (as well as some other minor nits).

Wonder if _Iterator should not be added to the eslint globals, as it seems to be provided by Loader.jsm ? Should be available in all modules ?

Comment 3

[Julian Descottes]

Attachment: Bug1184746.computedview.part3.v1.patch

Removed all named functions for consistency : 

myFunction: function myModule_myFunction() { 

-> 

myFunction: function() {}

Didn't see this pattern elsewhere and eslint wants camelCased names for functions.

Comment 4

[Julian Descottes]

Attachment: Bug1184746.computedview.part4.v1.patch

openStyleEditor method contained some dead code linked to a never used contentDoc variable.

Comment 5

[Julian Descottes]

Attachment: Bug1184746.computedview.part5.v1.patch

(not related to eslint)

Promise was imported as promise (lowercase). 
For consistency with other modules, use Promise.

Comment 6

[Julian Descottes]

Attachment: Bug1184746.computedview.part6.v1.patch

Consistent return in refreshMatchedSelectors.

Instead of doing a return; in the middle of a promise that should return a promise, return Promise.resolve().

To satisfy the no-else-return rule, the return value was also extracted to a "promise" variable.

Comment 7

[Julian Descottes]

Attachment: Bug1184746.computedview.part7.v1.patch

Similar to previous patch. Return Promise.resolve() instead of return; for return consistency.

Comment 8

[Julian Descottes]

Just uploaded attachments for a first pass on computed-view. 

I deliberately didn't follow two rules too strictly : 

- max length 80 char. Not sure how hard we want to enforce this. It's a good reminder to avoid long, complicated lines. Here I applied it where I felt readability was bad, but skipped it otherwise.

- no-unused-var , for method arguments. Even if an argument is not used, I think it's good to keep it in the method signature if we know it is provided when the method is called (such as the event object for an event handler).

Comment 9

[Julian Descottes]

Attachment: Bug1184746.ruleview.part1.v1.patch

Import Promise as "Promise" instead of "promise"

Comment 10

[Julian Descottes]

Attachment: Bug1184746.ruleview.part2.v1.patch

Rule view : return consistency. Mostly return Promise.resolve(undefined); instead of return; when expecting a promise.

Comment 11

[Julian Descottes]

Attachment: Bug1184746.ruleview.part3.v1.patch

Extract function to avoid arguments shadowing arguments from upper scope.

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8637456 [details] [diff] [review]
Bug1184746.ruleview.part1.v1.patch

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

I believe this change would be conflicting with natively implemented DOM Promise.

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8637461 [details] [diff] [review]
Bug1184746.ruleview.part3.v1.patch

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

::: browser/devtools/styleinspector/rule-view.js
@@ +1933,5 @@
>    },
>  
> +  /**
> +   * Toggle the visibility of an expandable container
> +   * @param  {DOMNode}  twisty     clickable toggle element

We have some inconsistencies with how our jsdocs are formatted. Taking a quick look majority of them have the description on a new line.

Can you make sure the description is on a new line and align with the object type of the param?

Comment 14

[Gabriel Luong [:gl] (ΦωΦ)]

The commit messages need to be formatted correctly. I only managed to find an old version of the docs, but what is said still holds true. https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#Commit_Message_Conventions

The commit msg should have the following format:
Bug 123456 - <Summary> r=mikeratcliffe

Try to be a bit more descriptive with the summary of the bugs. For instance, part 1 of computed view could be: "Corrected formatting of function braces in computed-view.js"

Since we have multiple parts, you will also want to format it as the following: "Bug 123456 - Part 1: <Summary> r=mikeracliffe"

Comment 15

[Julian Descottes]

> I believe this change would be conflicting with natively implemented DOM Promise. 

Looking at other modules in the devtools, Promise is sometimes imported as "promise", sometimes as "Promise". Is it really an issue to shadow the DOM Promise here ? 

> Can you make sure the description is on a new line and align with the object type of the param? 

Ok will do.

> The commit messages need to be formatted correctly

I wanted to have small commits for review, and after the review, squash them and format them correctly. I don't think this bug is worth having 10 commits in the repository ?

Comment 16

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Julian Descottes from comment #15)
> > I believe this change would be conflicting with natively implemented DOM Promise. 
> 
> Looking at other modules in the devtools, Promise is sometimes imported as
> "promise", sometimes as "Promise". Is it really an issue to shadow the DOM
> Promise here ? 

Native implementation of DOM Promises are more recent. I suspect that is the reason for the disparity. I would defer to Mike's judgement here. I believe we shouldn't try to shadow native objects. In this case, we want to be clear if we are using DOM Promises or jsm promises.

> > Can you make sure the description is on a new line and align with the object type of the param? 
> 
> Ok will do.
> 
> > The commit messages need to be formatted correctly
> 
> I wanted to have small commits for review, and after the review, squash them
> and format them correctly. I don't think this bug is worth having 10 commits
> in the repository ?

From what I have seen, I would say it would be safe to have one big patch for all the eslint cleanup since the review for those are very quick. If you're refactoring a particular function, I would move that to a separate patch.

Comment 17

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Julian Descottes from comment #8)
> Just uploaded attachments for a first pass on computed-view. 
> 
> I deliberately didn't follow two rules too strictly : 
> 
> - max length 80 char. Not sure how hard we want to enforce this. It's a good
> reminder to avoid long, complicated lines. Here I applied it where I felt
> readability was bad, but skipped it otherwise.
> 
> - no-unused-var , for method arguments. Even if an argument is not used, I
> think it's good to keep it in the method signature if we know it is provided
> when the method is called (such as the event object for an event handler).

I believe the eventual goal is to have the try build process also run eslint and warn of lint errors. We should try to fix all the eslint errors the best we can.

BTW, thanks for the work so far! Don't hesitate to ask for feedback or review early.

Comment 18

[Julian Descottes]

@Gabriel : thanks for having a look at the patches ! I integrated your comments, just waiting for Mike's review before reuploading.

> We should try to fix all the eslint errors the best we can.

The two I mentioned are only warnings, so maybe we can leave them for now ? 

When choosing the eslint configuration, did you discuss about the unused-var rule applied to function arguments ? (found one PR discussing the topic on eslint's repo: https://github.com/eslint/eslint/issues/1939)

Comment 19

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Julian Descottes from comment #18)
> @Gabriel : thanks for having a look at the patches ! I integrated your
> comments, just waiting for Mike's review before reuploading.
> 
> > We should try to fix all the eslint errors the best we can.
> 
> The two I mentioned are only warnings, so maybe we can leave them for now ? 
> 
> When choosing the eslint configuration, did you discuss about the unused-var
> rule applied to function arguments ? (found one PR discussing the topic on
> eslint's repo: https://github.com/eslint/eslint/issues/1939)

I think eslint errors would be the first priority. I know there have been eslint discussions within the devtools team and should be noted in some bugs in which the configurations were committed. Regarding leaving eslint warnings, I would wait on Mike's review. I suspect it will be fine to keep eslint warnings, but there will always be that minor annoyance of seeing the lint warning and somebody will eventually try to fix it.

That said I believe there are also some inconsistencies already with keeping the unused event variable as well. So, it would be good to determine what we should do about it.

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Just to let you know: I would have been more than happy to review this as a single patch as it is fairly short.

Comment 21

[Julian Descottes]

@Mike : Sorry, I was afraid I was splitting too much this time ... Thanks for the review in any case.

What do you think about importing Promise.jsm as Promise instead of promise, is it fine with you ?
Gabriel was concerned about overshadowing the DOM Promise implementation.

I'll push this to try later today.

Comment 22

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I thought the same thing... seeing it used that way can easily confuse new contributors.

Lets import it as promise and use it that way throughout your patches... you don't need to ask for review for that change though, just make sure it runs through try.

Comment 23

[Julian Descottes]

Attachment: Bug1184746.try.v1.patch

Try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc065334412d

Comment 24

[Gabriel Luong [:gl] (ΦωΦ)]

Julian, I think it is safe to set the "checkin-needed" flag in the "keywords" judging from the try build results.

Comment 25

[Gabriel Luong [:gl] (ΦωΦ)]

The commit message just needs one adjustment (change : to -):
Bug 1184746 : <Msg> to Bug 1184746 - <Msg>

Comment 26

[Julian Descottes]

Ok, will do later today.

Comment 27

[Julian Descottes]

Attachment: Bug1184746.try.v2.patch

Updated commit comment.

Comment 28

[Julian Descottes]

Attachment: Bug1184746.try.v3.patch

Comment 29

[Julian Descottes]

Attachment: Bug1184746.try.v4.patch

Comment 30

[Julian Descottes]

Do not pay attention to v2 and v3, both were wrong uploads.

Comment 31

[Gabriel Luong [:gl] (ΦωΦ)]

I went ahead and committed your changes Julian.

Good work and thank you for the patch!

https://hg.mozilla.org/integration/fx-team/rev/ffdd06074331

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ffdd06074331