Closed Bug 1184746 Opened 5 years ago Closed 4 years ago

Eslint cleanup for rule-view & computed-view

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: julian.descottes, Assigned: julian.descottes)

Details

Attachments

(1 file, 13 obsolete files)

38.27 KB, patch
julian.descottes
: review+
Details | Diff | Splinter Review
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.
Component: Untriaged → Developer Tools: Inspector
OS: Unspecified → All
Hardware: Unspecified → All
Summary: Devtools style-inspector : eslint cleanup for rule-view & computed-view → Eslint cleanup for rule-view & computed-view
Only minor nits in this one : curly braces after line return, missing spaces ...
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 ?
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.
openStyleEditor method contained some dead code linked to a never used contentDoc variable.
(not related to eslint)

Promise was imported as promise (lowercase). 
For consistency with other modules, use Promise.
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.
Similar to previous patch. Return Promise.resolve() instead of return; for return consistency.
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).
Assignee: nobody → julian.descottes
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Import Promise as "Promise" instead of "promise"
Rule view : return consistency. Mostly return Promise.resolve(undefined); instead of return; when expecting a promise.
Extract function to avoid arguments shadowing arguments from upper scope.
Attachment #8634992 - Flags: review?(mratcliffe)
Attachment #8634993 - Flags: review?(mratcliffe)
Attachment #8634994 - Flags: review?(mratcliffe)
Attachment #8634995 - Flags: review?(mratcliffe)
Attachment #8635002 - Flags: review?(mratcliffe)
Attachment #8635003 - Flags: review?(mratcliffe)
Attachment #8635004 - Flags: review?(mratcliffe)
Attachment #8637456 - Flags: review?(mratcliffe)
Attachment #8637460 - Flags: review?(mratcliffe)
Attachment #8637461 - Flags: review?(mratcliffe)
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.
Attachment #8637456 - Flags: feedback+
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?
Attachment #8637461 - Flags: feedback+
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"
> 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 ?
(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.
(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.
@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)
(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.
Attachment #8634992 - Flags: review?(mratcliffe) → review+
Attachment #8634993 - Flags: review?(mratcliffe) → review+
Attachment #8634994 - Flags: review?(mratcliffe) → review+
Attachment #8634995 - Flags: review?(mratcliffe) → review+
Attachment #8635002 - Flags: review?(mratcliffe) → review+
Attachment #8635003 - Flags: review?(mratcliffe) → review+
Attachment #8635004 - Flags: review?(mratcliffe) → review+
Attachment #8637456 - Flags: review?(mratcliffe) → review+
Attachment #8637460 - Flags: review?(mratcliffe) → review+
Attachment #8637461 - Flags: review?(mratcliffe) → review+
Just to let you know: I would have been more than happy to review this as a single patch as it is fairly short.
@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.
Flags: needinfo?(mratcliffe)
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.
Flags: needinfo?(mratcliffe)
Attached patch Bug1184746.try.v1.patch (obsolete) — Splinter Review
Try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc065334412d
Attachment #8634992 - Attachment is obsolete: true
Attachment #8634993 - Attachment is obsolete: true
Attachment #8634994 - Attachment is obsolete: true
Attachment #8634995 - Attachment is obsolete: true
Attachment #8635002 - Attachment is obsolete: true
Attachment #8635003 - Attachment is obsolete: true
Attachment #8635004 - Attachment is obsolete: true
Attachment #8637456 - Attachment is obsolete: true
Attachment #8637460 - Attachment is obsolete: true
Attachment #8637461 - Attachment is obsolete: true
Attachment #8638183 - Flags: review+
Julian, I think it is safe to set the "checkin-needed" flag in the "keywords" judging from the try build results.
Keywords: checkin-needed
The commit message just needs one adjustment (change : to -):
Bug 1184746 : <Msg> to Bug 1184746 - <Msg>
Keywords: checkin-needed
Ok, will do later today.
Attached patch Bug1184746.try.v2.patch (obsolete) — Splinter Review
Updated commit comment.
Attachment #8638183 - Attachment is obsolete: true
Attachment #8638834 - Flags: review+
Attached patch Bug1184746.try.v3.patch (obsolete) — Splinter Review
Attachment #8638834 - Attachment is obsolete: true
Attachment #8638836 - Flags: review+
Attachment #8638836 - Attachment is obsolete: true
Attachment #8638837 - Flags: review+
Do not pay attention to v2 and v3, both were wrong uploads.
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/ffdd06074331
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.