Closed
Bug 1184746
Opened 9 years ago
Closed 9 years ago
Eslint cleanup for rule-view & computed-view
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
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.
Updated•9 years ago
|
Component: Untriaged → Developer Tools: Inspector
Updated•9 years ago
|
OS: Unspecified → All
Hardware: Unspecified → All
Updated•9 years ago
|
Summary: Devtools style-inspector : eslint cleanup for rule-view & computed-view → Eslint cleanup for rule-view & computed-view
Assignee | ||
Comment 1•9 years ago
|
||
Only minor nits in this one : curly braces after line return, missing spaces ...
Assignee | ||
Comment 2•9 years ago
|
||
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 ?
Assignee | ||
Comment 3•9 years ago
|
||
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.
Assignee | ||
Comment 4•9 years ago
|
||
openStyleEditor method contained some dead code linked to a never used contentDoc variable.
Assignee | ||
Comment 5•9 years ago
|
||
(not related to eslint) Promise was imported as promise (lowercase). For consistency with other modules, use Promise.
Assignee | ||
Comment 6•9 years ago
|
||
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.
Assignee | ||
Comment 7•9 years ago
|
||
Similar to previous patch. Return Promise.resolve() instead of return; for return consistency.
Assignee | ||
Comment 8•9 years ago
|
||
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).
Updated•9 years ago
|
Assignee: nobody → julian.descottes
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 9•9 years ago
|
||
Import Promise as "Promise" instead of "promise"
Assignee | ||
Comment 10•9 years ago
|
||
Rule view : return consistency. Mostly return Promise.resolve(undefined); instead of return; when expecting a promise.
Assignee | ||
Comment 11•9 years ago
|
||
Extract function to avoid arguments shadowing arguments from upper scope.
Assignee | ||
Updated•9 years ago
|
Attachment #8634992 -
Flags: review?(mratcliffe)
Assignee | ||
Updated•9 years ago
|
Attachment #8634993 -
Flags: review?(mratcliffe)
Assignee | ||
Updated•9 years ago
|
Attachment #8634994 -
Flags: review?(mratcliffe)
Assignee | ||
Updated•9 years ago
|
Attachment #8634995 -
Flags: review?(mratcliffe)
Assignee | ||
Updated•9 years ago
|
Attachment #8635002 -
Flags: review?(mratcliffe)
Assignee | ||
Updated•9 years ago
|
Attachment #8635003 -
Flags: review?(mratcliffe)
Assignee | ||
Updated•9 years ago
|
Attachment #8635004 -
Flags: review?(mratcliffe)
Assignee | ||
Updated•9 years ago
|
Attachment #8637456 -
Flags: review?(mratcliffe)
Assignee | ||
Updated•9 years ago
|
Attachment #8637460 -
Flags: review?(mratcliffe)
Assignee | ||
Updated•9 years ago
|
Attachment #8637461 -
Flags: review?(mratcliffe)
Comment 12•9 years ago
|
||
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 13•9 years ago
|
||
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+
Comment 14•9 years ago
|
||
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"
Assignee | ||
Comment 15•9 years ago
|
||
> 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•9 years ago
|
||
(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•9 years ago
|
||
(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.
Assignee | ||
Comment 18•9 years ago
|
||
@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•9 years ago
|
||
(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.
Assignee | ||
Comment 21•9 years ago
|
||
@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)
Assignee | ||
Comment 23•9 years ago
|
||
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+
Comment 24•9 years ago
|
||
Julian, I think it is safe to set the "checkin-needed" flag in the "keywords" judging from the try build results.
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 25•9 years ago
|
||
The commit message just needs one adjustment (change : to -): Bug 1184746 : <Msg> to Bug 1184746 - <Msg>
Keywords: checkin-needed
Assignee | ||
Comment 26•9 years ago
|
||
Ok, will do later today.
Assignee | ||
Comment 27•9 years ago
|
||
Updated commit comment.
Attachment #8638183 -
Attachment is obsolete: true
Attachment #8638834 -
Flags: review+
Assignee | ||
Comment 28•9 years ago
|
||
Attachment #8638834 -
Attachment is obsolete: true
Attachment #8638836 -
Flags: review+
Assignee | ||
Comment 29•9 years ago
|
||
Attachment #8638836 -
Attachment is obsolete: true
Attachment #8638837 -
Flags: review+
Assignee | ||
Comment 30•9 years ago
|
||
Do not pay attention to v2 and v3, both were wrong uploads.
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Updated•9 years ago
|
Keywords: checkin-needed
Comment 31•9 years ago
|
||
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•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ffdd06074331
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•