Enable eslint rules for Loop: No unused variables

RESOLVED FIXED in Firefox 45

Status

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

People

(Reporter: standard8, Assigned: standard8, Mentored)

Tracking

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

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

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Enable some more eslint rules for Loop. Rules to enable in this bug:

* no-unused-vars - disallows unused variables

To enable the 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 .js file that has an associated .jsx file, then fix the jsx 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/

Updated

3 years ago
Rank: 35
Priority: -- → P3

Comment 1

3 years ago
Hi
Can you please assign this bug to me?
(Assignee)

Comment 2

3 years ago
Sure, let me know if you need help.
Assignee: nobody → hussain237

Comment 3

3 years ago
Thanks
Can you please guide me how to 
   
Change 
c:\Users\moztest\AppData\Roaming

to /c/Users/moztest/AppData/Roaming
on widows platform
(Assignee)

Comment 4

3 years ago
(In reply to Hussain from comment #3)
> Thanks
> Can you please guide me how to 
>    
> Change 
> c:\Users\moztest\AppData\Roaming

You need to enter the following on the console:

  echo $APPDATA

Then in whatever that prints out, you need to change the '\' to '/', add one '/' at the beginning and drop the ':'.

Comment 5

3 years ago
Hi
I have use the 
echo $PATH:/c/program\ files/nodejs:/c/Users/hussain/AppData/Roaming/npm >> ~/.profile

But I get error following error when i use npm install -g react-tools@0.12.2 command 
bash: npm: command not found
(Assignee)

Comment 6

3 years ago
(In reply to Hussain from comment #5)
> Hi
> I have use the 
> echo $PATH:/c/program\ files/nodejs:/c/Users/hussain/AppData/Roaming/npm >>
> ~/.profile
> 
> But I get error following error when i use npm install -g react-tools@0.12.2
> command 
> bash: npm: command not found

Did you restart the shell/command prompt after doing this?

Comment 7

3 years ago
Yes I did restart shell. Is there another way to address this problem
(Assignee)

Comment 8

3 years ago
I've just realised that the wiki page is wrong. I've now corrected it. Sorry about that. 

What you'll need to do is to edit ~/.profile, and change:

$PATH:/c/program\ files/nodejs:/c/Users/hussain/AppData/Roaming/npm

to:

PATH=$PATH:/c/program\ files/nodejs:/c/Users/hussain/AppData/Roaming/npm

and then restart the command prompt again. That should hopefully fix it.

Comment 9

3 years ago
Thank you for your help
Just an other other are these two action optional?

/path/to/firefox -createProfile myTestProfile
./mach run -P myTestProfile -purgecaches

 Also is  /path/to/firefox mean firefox installed on machine or firefox folder in mozilla-central

Thanks
(Assignee)

Comment 10

3 years ago
(In reply to Hussain from comment #9)
> Thank you for your help
> Just an other other are these two action optional?
> 
> /path/to/firefox -createProfile myTestProfile

This is used to create a new test profile. You don't have to create a separate one, but it keeps your testing/development separate from your main one.

> ./mach run -P myTestProfile -purgecaches

You can do just

./mach run -purgecaches

to start Firefox with the default profile.

>  Also is  /path/to/firefox mean firefox installed on machine or firefox
> folder in mozilla-central

It would the the one installed on your machine.
(Assignee)

Comment 11

3 years ago
Hussain, are you still willing/able to work on this? Please let us know either way.
Flags: needinfo?(hussain237)
(Assignee)

Comment 12

3 years ago
Sorry Hussain, its been a few weeks now and I haven't heard anything. We want to move this bug forward to help the project, so I'm going to take it. If you want to work on other bugs in the future, please do. This one has become a slightly higher priority as we need to be cleaning our code better.
Assignee: hussain237 → standard8
Flags: needinfo?(hussain237)
(Assignee)

Comment 13

3 years ago
Created attachment 8696594 [details] [diff] [review]
No unused variables.
Attachment #8696594 - Flags: review?(mdeboer)
Attachment #8696594 - Flags: review?(edilee)
Attachment #8696594 - Flags: review?(dmose)

Comment 14

3 years ago
Comment on attachment 8696594 [details] [diff] [review]
No unused variables.

Review of attachment 8696594 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/extensions/loop/content/shared/js/linkifiedTextView.jsx
@@ -4,5 @@
>  
>  var loop = loop || {};
>  loop.shared = loop.shared || {};
>  loop.shared.views = loop.shared.views || {};
> -loop.shared.views.LinkifiedTextView = (function(mozL10n) {

Do we want to get rid of this? Might be nice to consistently expect mozL10n.

::: browser/extensions/loop/content/shared/js/otSdkDriver.js
@@ -991,4 @@
>       * which to copy the stream when attaching it to visible video element
>       * that the views control directly.
>       *
>       * @param event {OT.VideoEnabledChangedEvent} from the SDK

@param event comment should be removed

@@ -1011,4 @@
>       * Handle the SDK disabling of remote video by dispatching the
>       * appropriate event.
>       *
>       * @param event {OT.VideoEnabledChangedEvent) from the SDK

same @param event
Attachment #8696594 - Flags: review?(mdeboer)
Attachment #8696594 - Flags: review?(edilee)
Attachment #8696594 - Flags: review?(dmose)
Attachment #8696594 - Flags: review+
(Assignee)

Comment 15

3 years ago
(In reply to Ed Lee :Mardak from comment #14)
> > -loop.shared.views.LinkifiedTextView = (function(mozL10n) {
> 
> Do we want to get rid of this? Might be nice to consistently expect mozL10n.

Well, we can always add it back later ;-) Though I did look at that component and think it unlikely that we'll ever need l10n in there due to its limited scope.

Comment 17

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/caf389bbafbc
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Assignee)

Updated

3 years ago
Blocks: 1232994
You need to log in before you can comment on or make changes to this bug.