Enable eslint of browser/components/translation/

RESOLVED FIXED in Firefox 52

Status

()

Firefox
Translation
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: standard8, Assigned: jordan, Mentored)

Tracking

unspecified
Firefox 52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [lang=js])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

8 months ago
To help with preventing errors in coding, we should enable eslint in browser/components/translation/

To enable checking of files, remove the `browser/components/translation/**` entry from the top-level `.eslintignore`.

Then you can run eslint on the directory with

  ./mach eslint browser/components/translation/

(note: eslint needs a one-time setup of `./mach eslint --setup`).

The errors listed in the output are the ones to be fixed.

Information about the eslint rules can be found here:

http://eslint.org/docs/rules/

If you are uncertain of how to fix a rule, please feel free to ask here or on irc (https://wiki.mozilla.org/Irc) in #fx-team (I'm on as Standard8).
(Assignee)

Comment 1

8 months ago
Mark, I'd like to be assigned to this bug, could you assign me please.
(Reporter)

Comment 2

8 months ago
Hi Sourav, thank you for picking this up. You're now assigned :-)
Assignee: nobody → souravgarg833
(Assignee)

Comment 3

8 months ago
hi Mark, i'm getting this error while running eslint on browser/components/translation/

sunny mozilla-central$ ./mach eslint browser/components/translation/
An error occurred running eslint. Please check the following error messages:

Invalid string length
RangeError: Invalid string length
    at Object.stringify (native)
    at module.exports (/Users/sunny/mozilla-central/tools/lint/eslint/node_modules/eslint/lib/formatters/json.js:12:17)
    at printResults (/Users/sunny/mozilla-central/tools/lint/eslint/node_modules/eslint/lib/cli.js:80:20)
    at Object.execute (/Users/sunny/mozilla-central/tools/lint/eslint/node_modules/eslint/lib/cli.js:186:17)
    at Object.<anonymous> (/Users/sunny/mozilla-central/tools/lint/eslint/node_modules/eslint/bin/eslint.js:76:28)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Function.Module.runMain (module.js:441:10)
✖ 0 problems (0 errors, 0 warnings)



I don't have much clue what is it about. I asked out in irc but nothing helped. 
You may wanna refer https://github.com/eslint/eslint/issues/5380
Thanks!
(Reporter)

Comment 4

8 months ago
Hi Sourav

(In reply to Sourav Garg [:jordan] from comment #3)
> hi Mark, i'm getting this error while running eslint on
> browser/components/translation/
> 
> sunny mozilla-central$ ./mach eslint browser/components/translation/
> An error occurred running eslint. Please check the following error messages:

Which line(s) did you remove from .eslintignore? Currently there's these three lines:

    browser/components/translation/**
    # generated files in cld2
    browser/components/translation/cld2/cld-worker.js

You only need to remove the first one, the last one (and the comment) is still needed as its a generated file so we won't lint that.
Comment hidden (mozreview-request)

Comment 6

8 months ago
mozreview-review
Comment on attachment 8804885 [details]
Bug 1311349 - Enable eslint of browser/components/translation/;

https://reviewboard.mozilla.org/r/88716/#review87824

Thanks for looking at this :-)

::: browser/components/translation/TranslationDocument.jsm:288
(Diff revision 1)
> +      else {
> +        rootType = ' (non simple root)';
> +      }
> +    }
> +    else {
> +      rootType = '';

nit: You don't need this else block, just initialize rootType to ''.

Also, it's not clear why this function mixes double and single quotes, can we take this change as an opportunity to make all strings in the function use double quotes?

::: browser/components/translation/test/browser_translation_bing.js:1
(Diff revision 1)
> +

Is there a reason for adding an empty line before the license header here?

::: browser/components/translation/test/browser_translation_telemetry.js:120
(Diff revision 1)
>  var translate = Task.async(function*(text, from, closeTab = true) {
>    let tab = yield offerTranslationFor(text, from);
>    yield acceptTranslationOffer(tab);
>    if (closeTab) {
>      gBrowser.removeTab(tab);
> -  } else {
> +    return '';

Given that the returned 'tab' value in the other case seems to be an object, I think it would make more sense to return null here rather than an empty string.
Comment hidden (mozreview-request)
(Assignee)

Comment 8

8 months ago
Hi Florian, I made the necessary changes. Please have a relook on the updated attachment. Thanks!

Comment 9

8 months ago
mozreview-review
Comment on attachment 8804885 [details]
Bug 1311349 - Enable eslint of browser/components/translation/;

https://reviewboard.mozilla.org/r/88716/#review88034

Looks good to me, thanks!
Attachment #8804885 - Flags: review+
(Reporter)

Updated

8 months ago
Attachment #8804885 - Flags: review?(standard8)
(Reporter)

Comment 10

8 months ago
mozreview-review
Comment on attachment 8804885 [details]
Bug 1311349 - Enable eslint of browser/components/translation/;

https://reviewboard.mozilla.org/r/88716/#review88036

Adding my r+ as well to make mozreview happy.
Attachment #8804885 - Flags: review+
(Reporter)

Comment 11

8 months ago
Hi Sourav, as you have r+ with no issues left, its now complete. Not sure if this is your first patch or not, so in case it isn't:

Normally you'd add checkin-needed as a keyword at the top of the bug, and then someone would come along and land it for you a little later. However, as I'm here, I'll skip that part and get it landed for you now - it'll first go into one of our integration repositories (and the bug will be updated), then within about 24 hours it'll be merged into our main development repository and the bug will then be marked fixed.

Thank you for working on this, I'm glad you got your repository issue sorted out.

Comment 12

8 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0c8e6757abd6
Enable eslint of browser/components/translation/; r=florian,standard8

Comment 13

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0c8e6757abd6
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.