Closed
Bug 1170757
Opened 10 years ago
Closed 10 years ago
Enable eslint rules for Loop: Formatting related
Categories
(Hello (Loop) :: Client, defect, P3)
Hello (Loop)
Client
Tracking
(firefox42 fixed)
People
(Reporter: standard8, Assigned: frsela, Mentored)
References
Details
(Whiteboard: [lang=js][tech-debt])
Attachments
(1 file, 5 obsolete files)
21.74 KB,
patch
|
dmosedale
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•10 years ago
|
Assignee: nobody → frsela
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8636476 -
Flags: review?(standard8)
Reporter | ||
Comment 2•10 years ago
|
||
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-
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8636476 -
Attachment is obsolete: true
Attachment #8638419 -
Flags: review?(standard8)
Assignee | ||
Comment 4•10 years ago
|
||
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)
Reporter | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8638529 -
Attachment is obsolete: true
Attachment #8639205 -
Flags: review?(standard8)
Assignee | ||
Updated•10 years ago
|
Attachment #8639205 -
Flags: review?(standard8) → review?(dmose)
Comment 7•10 years ago
|
||
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)
Comment 8•10 years ago
|
||
Attachment #8639205 -
Attachment is obsolete: true
Attachment #8639605 -
Flags: review?(dmose)
Comment 9•10 years ago
|
||
Please reset the review flag on me once you've had a chance to respond to the questions; thanks!
Flags: needinfo?(frsela)
Updated•10 years ago
|
Attachment #8639605 -
Flags: review?(dmose)
Assignee | ||
Comment 10•10 years ago
|
||
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)
Reporter | ||
Comment 11•10 years ago
|
||
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)
Comment 12•10 years ago
|
||
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+
Comment 13•10 years ago
|
||
(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.
Comment 14•10 years ago
|
||
And landed on fx-team. Sorry for the delay!
Comment 16•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Updated•10 years ago
|
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.
Description
•