Enable eslint rules for Loop: Formatting related

RESOLVED FIXED in Firefox 42

Status

P3
normal
Rank:
30
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: standard8, Assigned: frsela, Mentored)

Tracking

unspecified
mozilla42
Points:
---
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox42 fixed)

Details

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

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

3 years ago
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
Created attachment 8636476 [details] [diff] [review]
Bug1170757.patch
Attachment #8636476 - Flags: review?(standard8)
(Reporter)

Comment 2

3 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-
Created attachment 8638419 [details] [diff] [review]
Bug1170757.patch
Attachment #8636476 - Attachment is obsolete: true
Attachment #8638419 - Flags: review?(standard8)
Created attachment 8638529 [details] [diff] [review]
Bug1170757.patch

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

3 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)
Created attachment 8639205 [details] [diff] [review]
Bug1170757.patch
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)
Created attachment 8639605 [details] [diff] [review]
enabled eslint formatting rules for Hello
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)
(Reporter)

Comment 11

3 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)
Created attachment 8643843 [details] [diff] [review]
Enabled eslint dot-location + no-empty rules for Hello

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!
https://hg.mozilla.org/mozilla-central/rev/e72e9e0222d8
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42

Updated

3 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.