Closed Bug 1416149 Opened 7 years ago Closed 4 years ago

Switch from test to lint job for checking fluent files

Categories

(Core :: Internationalization: Localization, enhancement, P2)

Product:
Core
Component:
Internationalization: Localization
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
87 Branch
Tracking Flags:
Tracking Status
firefox87 --- fixed

People

(Reporter: flod, Assigned: dminor)

References

(Blocks 3 open bugs)

Details

Attachments

(2 files)

Bug 1416149 - Add linter for checking fluent files
(deleted), text/x-phabricator-request
Details
Bug 1416149 - Fix placement of test_fluent_lint.py
(deleted), text/x-phabricator-request
Details
.properties and .dtd are covered by https://searchfox.org/mozilla-central/source/browser/base/content/test/static/browser_misused_characters_in_strings.js Only listed exceptions are allowed to use ' and ".
First thought - should it be a pre-commit hook instead of a test?
I have the feeling tests is the right way to go, but I have a very limited picture. Aren't pre-commit hooks expensive? To add some reference: the current test was added in bug 1259859.
> Aren't pre-commit hooks expensive? In the long run, tests are more expensive, because hook runs once per patch, test runs per each test run. Also hook runs on the strings affected by a commit, test runs on all strings, forever. But I'm not the best person to judge, just wanted to throw an idea.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #3) > > Aren't pre-commit hooks expensive? > > In the long run, tests are more expensive, because hook runs once per patch, > test runs per each test run. Also hook runs on the strings affected by a > commit, test runs on all strings, forever. I think this makes sense, fwiw. I forget why we went with a test, maybe because it was just easier to implement straightaway, also with a list of current exceptions etc, which might be a bit ugly in a push/commit hook. > But I'm not the best person to judge, just wanted to throw an idea. Might be worth checking with some build/infra folks. ISTR I recently got told that we're trying to stop having quite as many push hooks, though I can't remember the context or who told me. :-( Pre-commit hooks as things client-side I'd be worried about in the sense that I still see stuff getting backed out for eslint issues which, in theory, should all be getting caught by pre-commit hooks, and if not by that then by mozreview lintbots - but seems to still happen! Anyway, my $.02.
(In reply to Francesco Lodolo [:flod] from bug 1517496 comment #1) > We already have a bug on file (bug 1416149). I don't mind duping it one way > or the other. bug 1517496 has instructions on how to get this to work for fluent, so let's leave that for now to get it done ASAP so we have "something" in place for ftl files. I think this bug can be helpful if we make it about adding a lint job. This can be a follow-up, given that it's a little trickier than just updating the extant test. I think a lint job is preferable because we get phabricator and automation integration, and we're moving away from commit hooks for other things (or so I've been told), esp. for more time-intensive things like linting. The docs in https://firefox-source-docs.mozilla.org/tools/lint/create.html provide a starting point. I believe it should be relatively straightforward to leverage the existing python parser support that's in the tree anyway from within a python linter file. If we create a mozlint job this way, we get automated phabricator reviews and automation job support for touched files for free.
Blocks: 1517304
Depends on: 1517496
Summary: Add test for misused characters in FTL files → Switch from test to lint job for checking fluent files
Priority: -- → P3
Assignee: nobody → berning5
Status: NEW → ASSIGNED

I see Fluent in the check file. Is this fixed now?

Flags: needinfo?(francesco.lodolo)

No. This is still a test, not a lint job. The fact that it uses Fluent was fixed by bug 1517496.

And, to be clear, I'm not the right person to decide if this should be closed as WONTFIX or kept open.

Flags: needinfo?(francesco.lodolo)
Component: Localization → Internationalization
Priority: P3 → --
Component: Internationalization → Internationalization: Localization

Additional note: the current solution doesn't seem to catch all strings (see the one in unknownContentType.ftl)
https://phabricator.services.mozilla.com/D100082

(I also doubt this bug is still assigned at this point)

Assignee: berning5 → nobody
Status: ASSIGNED → NEW
Priority: -- → P2

I'd probably be a good candidate to work on this once we prioritize it, as I have some experience working with the linting system.

I'm also interested in working on this :)

Some things that flod had to flag manually when reviewing Bug 1510797 that we might be able to lint:

  • Namespacing, e.g. common prefix for all messages in a single fluent file.
  • Whitespace around '##' and '###' block comments.
  • Presence and format of comments for variables used in a message.
  • Misuse of message references for recurrent text.

Namespacing might be challenging: old files don't use, some files already have very specific IDs without using a common namespace.
Same for message references: some are OK, so it would be really hard to avoid having too many errors.

We should enforce lowercase and - on IDs, but we also need to maintain a list of excecptions (per message, or entire files).

Greg and I are planning to work together on this.

Assignee: nobody → dminor

I have an initial version of this working. It includes the string checks from the existing test along with an identifier check. It is possible to disable the identifier check on a file by file basis. Try run here: https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=efrKIlHsT1miymEW8D6xcQ.0&revision=6c408b4a9424aea68b63771a68e9160ccecb7c7f.

Since Greg doesn't have time to get started on this right away, our plan was to get the initial version of this landed and then file follow on bugs to track adding more sophisticated checks.

This adds a linter for Fluent files based upon the existing test for bad
strings in browser_misused_characters_in_strings.js. It also adds a check
for identifiers that only permits lowercase letters, numbers and the
hyphen character (in ascii). Since a large number of existing identifiers
use uppercase letters, an exclusions file is used to disable the identifier
check on a file by file basis.

Blocks: 1691747

Depends on D104414

Pushed by dminor@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3a30710b35e8 Add linter for checking fluent files r=zbraniecki,gregtatum,flod,Gijs,fluent-reviewers,linter-reviewers,sylvestre
Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/b0085234bc44 Fix placement of test_fluent_lint.py r=gregtatum

https://hg.mozilla.org/mozilla-central/rev/3a30710b35e8
https://hg.mozilla.org/mozilla-central/rev/b0085234bc44

Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox87: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 87 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: