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

Mark, I'd like to be assigned to this bug, could you assign me please.

Hi Sourav, thank you for picking this up. You're now assigned :-)

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!

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

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

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!

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.

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.

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