Closed Bug 1170757 Opened 10 years ago Closed 10 years ago

Enable eslint rules for Loop: Formatting related

Categories

(Hello (Loop) :: Client, defect, P3)

defect

Tracking

(firefox42 fixed)

RESOLVED FIXED
mozilla42
Iteration:
42.3 - Aug 10
Tracking Status
firefox42 --- fixed
backlog tech-debt

People

(Reporter: standard8, Assigned: frsela, Mentored)

References

Details

(Whiteboard: [lang=js][tech-debt])

Attachments

(1 file, 5 obsolete files)

Enable some more eslint rules for Loop. Rules to enable in this bug: * dot-location - enforce consistent newlines before dot * no-empty - no empty statements To enable the dot-location rule, change the line in browser/components/loop/.eslintrc to be: "dot-location": [2, property], To enable the no-empty rule, remove the line from browser/components/loop/.eslintrc and save it. Then you can run eslint in the browser/components/loop directory with: eslint --ext .js --ext .jsm --ext .jsx . The errors listed in the output are to be fixed. If there's errors in a .jsx file that is affected: fix the file and run the react tools to generate the .js file (https://wiki.mozilla.org/Loop/Development#Developing). If you need more help setting up eslint, see https://wiki.mozilla.org/Loop/Development#Additional_Requirements Information about the eslint rules can be found here: http://eslint.org/docs/rules/
Assignee: nobody → frsela
Attached patch Bug1170757.patch (obsolete) — Splinter Review
Attachment #8636476 - Flags: review?(standard8)
Comment on attachment 8636476 [details] [diff] [review] Bug1170757.patch Review of attachment 8636476 [details] [diff] [review]: ----------------------------------------------------------------- This is heading in the right direction, but I think there's some further tidy up we can do here to improve the code and tests more. ::: browser/components/loop/.eslintrc @@ +37,5 @@ > // Eslint built-in rules are documented at <http://eslint.org/docs/rules/> > "camelcase": 0, // TODO: Remove (use default) > "computed-property-spacing": [2, "never"], > "consistent-return": 0, // TODO: Remove (use default) > + dot-location: [2, property], Although it didn't have quotes before, could you add quotes around the dot-location name. Also, I don't know where I got the [2, property] from, but according to the docs, that's wrong. Please just make it: "dot-location": 2, ::: browser/components/loop/content/js/roomStore.js @@ +524,5 @@ > var isValidURL = false; > try { > isValidURL = new URL(newRoomURL.location); > + } catch(ex) { > + false; Rather than false; can we make these a comment such as // URL may throw, default to false; According to the rules we should be ok doing it like this: http://eslint.org/docs/rules/no-empty ::: browser/components/loop/test/desktop-local/panel_test.js @@ +431,5 @@ > try { > TestUtils.findRenderedComponentWithType(view, loop.panel.ToSView); > sinon.assert.fail("Should not find the ToSView if it has been 'seen'"); > + } catch (ex) { > + false; We should change the test in these circumstances, this is much better and simpler as: expect(function() { TestUtils.findRenderedComponentWithType(view, loop.panel.ToSView); }).to.Throw(/not find/); Could you change the other similar tests to use this as well?
Attachment #8636476 - Flags: review?(standard8) → review-
Attached patch Bug1170757.patch (obsolete) — Splinter Review
Attachment #8636476 - Attachment is obsolete: true
Attachment #8638419 - Flags: review?(standard8)
Attached patch Bug1170757.patch (obsolete) — Splinter Review
Reverted as we chat at IRC this morning. Changed empty lines with comments instead "false;"
Attachment #8638419 - Attachment is obsolete: true
Attachment #8638419 - Flags: review?(standard8)
Attachment #8638529 - Flags: review?(standard8)
Comment on attachment 8638529 [details] [diff] [review] Bug1170757.patch I think this is the wrong version of the patch. Since I'm away next week, please request review from Dan.
Attachment #8638529 - Flags: review?(standard8)
Attached patch Bug1170757.patch (obsolete) — Splinter Review
Attachment #8638529 - Attachment is obsolete: true
Attachment #8639205 - Flags: review?(standard8)
Attachment #8639205 - Flags: review?(standard8) → review?(dmose)
Comment on attachment 8639205 [details] [diff] [review] Bug1170757.patch Review of attachment 8639205 [details] [diff] [review]: ----------------------------------------------------------------- Other than those two questions, this patch looks good. I'll upload a new version momentarily that's been rebased against the latest fx-team. ::: browser/components/loop/.eslintrc @@ +37,5 @@ > // Eslint built-in rules are documented at <http://eslint.org/docs/rules/> > "camelcase": 0, // TODO: Remove (use default) > "computed-property-spacing": [2, "never"], > "consistent-return": 0, // TODO: Remove (use default) > + dot-location: [2, property], This patch appears to not contain the change that Mark requested. But I get the impression from reading scrollback that you and Mark decided that this was the way to go. Is that true? If so, why? It also doesn't seem to have the quotes around the dot-location name. If that's intentional, it'd be helpful to know why.
Attachment #8639205 - Flags: review?(dmose)
Attachment #8639205 - Attachment is obsolete: true
Attachment #8639605 - Flags: review?(dmose)
Please reset the review flag on me once you've had a chance to respond to the questions; thanks!
Flags: needinfo?(frsela)
Attachment #8639605 - Flags: review?(dmose)
Hi Dan, (In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #7) > Comment on attachment 8639205 [details] [diff] [review] > Bug1170757.patch > > Review of attachment 8639205 [details] [diff] [review]: > ----------------------------------------------------------------- > > Other than those two questions, this patch looks good. I'll upload a new > version momentarily that's been rebased against the latest fx-team. > Thank you ! > ::: browser/components/loop/.eslintrc > @@ +37,5 @@ > > // Eslint built-in rules are documented at <http://eslint.org/docs/rules/> > > "camelcase": 0, // TODO: Remove (use default) > > "computed-property-spacing": [2, "never"], > > "consistent-return": 0, // TODO: Remove (use default) > > + dot-location: [2, property], > > This patch appears to not contain the change that Mark requested. But I get > the impression from reading scrollback that you and Mark decided that this > was the way to go. Is that true? If so, why? > Yes, we tested with dot-location: 2 and dot-location: [2, property] finally the last one was decided by Mark which is the same the "user-story" said. > It also doesn't seem to have the quotes around the dot-location name. If > that's intentional, it'd be helpful to know why. No, I forgot that, sorry. I can upload a new version if you want
Flags: needinfo?(frsela)
Comment on attachment 8639605 [details] [diff] [review] enabled eslint formatting rules for Hello re-adding review request per comment 9.
Attachment #8639605 - Flags: review?(dmose)
Fixed up syntax of dot-location line, added back no-empty due to the eslintrc semantic changes in eslint 1.0.
Attachment #8639605 - Attachment is obsolete: true
Attachment #8639605 - Flags: review?(dmose)
Attachment #8643843 - Flags: review+
(In reply to Fernando R. Sela (no CC, needinfo please) - PTO [:frsela] from comment #10) > > It also doesn't seem to have the quotes around the dot-location name. If > > that's intentional, it'd be helpful to know why. > > No, I forgot that, sorry. I can upload a new version if you want No worries, I've uploaded a version with that fix, as well as a change to cope with eslintrc semantic changes in eslint 1.0.
And landed on fx-team. Sorry for the delay!
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Iteration: --- → 42.3 - Aug 10
Rank: 30
Flags: firefox-backlog+
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: