Closed Bug 1206683 Opened 4 years ago Closed 4 years ago

Enable eslint rules for Loop: No unused variables

Categories

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

defect

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: standard8, Assigned: standard8, Mentored)

References

Details

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

Attachments

(1 file)

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/
Rank: 35
Priority: -- → P3
Hi
Can you please assign this bug to me?
Sure, let me know if you need help.
Assignee: nobody → hussain237
Thanks
Can you please guide me how to 
   
Change 
c:\Users\moztest\AppData\Roaming

to /c/Users/moztest/AppData/Roaming
on widows platform
(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 ':'.
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
(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?
Yes I did restart shell. Is there another way to address this problem
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.
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
(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.
Hussain, are you still willing/able to work on this? Please let us know either way.
Flags: needinfo?(hussain237)
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)
Attachment #8696594 - Flags: review?(mdeboer)
Attachment #8696594 - Flags: review?(edilee)
Attachment #8696594 - Flags: review?(dmose)
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+
(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.
https://hg.mozilla.org/mozilla-central/rev/caf389bbafbc
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Blocks: 1232994
You need to log in before you can comment on or make changes to this bug.