Closed Bug 1428769 Opened 2 years ago Closed 2 years ago

intl/l10n should be covered by eslint

Categories

(Core :: Internationalization, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: Pike, Assigned: zbraniecki)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Right now, all of intl/** is excluded from eslint, via the top-level .eslintignore.

We should fix that at least for the code we write newly in intl/l10n. The linter failures in intl/l10n/test are already pretty lengthy, with an intl/l10n/test/.eslintrc of

"use strict";

module.exports = {
    "extends": "plugin:mozilla/xpcshell-test"
};

Bug 1426063 adds more, so filing this.
Thanks for filing this Axel! I think it's blocked by bug 1393953 now. I was hoping to work on that cleanup after 0.5 release.
Depends on: 1393953
Priority: -- → P3
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #1)
> Thanks for filing this Axel! I think it's blocked by bug 1393953 now. I was
> hoping to work on that cleanup after 0.5 release.

I agree that that's necessary to lint all of intl/l10n, but that doesn't affect the code that's only in mozilla-central, like integration code and tests.
Depends on: 1439949
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
This is 99% https://github.com/projectfluent/fluent.js/commit/8e4f284e04b9333393da8c66dca46d82f3e60b57 and 1% test_l10nregistry.js alignment. :)

Depends on bug 1439949 to get async-iterators support in eslint.
Comment on attachment 8952737 [details]
Bug 1428769 - Add intl/l10n to be covered by eslint.

https://reviewboard.mozilla.org/r/221970/#review227834
Attachment #8952737 - Flags: review?(l10n) → review+
After bug 1439949 landed, we got some more eslint errors, which I just autofixed. I'll need to sync that back to fluent-gecko which will happen in bug 1434434.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 5c07bdfad5c24905680fddbb1abea0da9c157a80 -d a94434a6f077: rebasing 448327:5c07bdfad5c2 "Bug 1428769 - Add intl/l10n to be covered by eslint. r=Pike" (tip)
merging intl/l10n/L10nRegistry.jsm
merging intl/l10n/Localization.jsm
warning: conflicts while merging intl/l10n/L10nRegistry.jsm! (edit, then use 'hg resolve --mark')
warning: conflicts while merging intl/l10n/Localization.jsm! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2a34dd4ed3e3
Add intl/l10n to be covered by eslint. r=Pike
Comment on attachment 8952737 [details]
Bug 1428769 - Add intl/l10n to be covered by eslint.

https://reviewboard.mozilla.org/r/221970/#review228098


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: intl/l10n/Localization.jsm:256
(Diff revision 4)
>  
>    /**
>     * Register weak observers on events that will trigger cache invalidation
>     */
>    registerObservers() {
> -    ObserverService.addObserver(this, 'intl:app-locales-changed', true);
> +    Services.obs.addObserver(this, 'intl:app-locales-changed', true);

Error: Strings must use doublequote. [eslint: quotes]

::: intl/l10n/Localization.jsm:263
(Diff revision 4)
>  
>    /**
>     * Unregister observers on events that will trigger cache invalidation
>     */
>    unregisterObservers() {
> -    ObserverService.removeObserver(this, 'intl:app-locales-changed');
> +    Services.obs.removeObserver(this, 'intl:app-locales-changed');

Error: Strings must use doublequote. [eslint: quotes]
https://hg.mozilla.org/mozilla-central/rev/2a34dd4ed3e3
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.