Switch from test to lint job for checking fluent files
Categories
(Core :: Internationalization: Localization, enhancement, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox87 | --- | fixed |
People
(Reporter: flod, Assigned: dminor)
References
(Blocks 3 open bugs)
Details
Attachments
(2 files)
.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 ".
Comment 1•7 years ago
|
||
First thought - should it be a pre-commit hook instead of a test?
Reporter | ||
Comment 2•7 years ago
|
||
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.
Comment 3•7 years ago
|
||
> 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.
Comment 5•6 years ago
|
||
(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.
Comment 6•5 years ago
|
||
(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.
Updated•5 years ago
|
Updated•5 years ago
|
Comment 7•4 years ago
|
||
I see Fluent in the check file. Is this fixed now?
Reporter | ||
Comment 8•4 years ago
|
||
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.
Reporter | ||
Updated•4 years ago
|
Updated•3 years ago
|
Reporter | ||
Comment 9•3 years ago
|
||
Additional note: the current solution doesn't seem to catch all strings (see the one in unknownContentType.ftl)
https://phabricator.services.mozilla.com/D100082
Reporter | ||
Comment 10•3 years ago
|
||
(I also doubt this bug is still assigned at this point)
Updated•3 years ago
|
Comment 11•3 years ago
|
||
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.
Assignee | ||
Comment 12•3 years ago
|
||
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.
Reporter | ||
Comment 13•3 years ago
|
||
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).
Assignee | ||
Comment 14•3 years ago
|
||
Greg and I are planning to work together on this.
Assignee | ||
Comment 15•3 years ago
|
||
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.
Assignee | ||
Comment 16•3 years ago
|
||
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.
Assignee | ||
Comment 17•3 years ago
|
||
Assignee | ||
Comment 18•3 years ago
|
||
Depends on D104414
Comment 19•3 years ago
|
||
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
Comment 20•3 years ago
|
||
Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/b0085234bc44 Fix placement of test_fluent_lint.py r=gregtatum
Comment 21•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3a30710b35e8
https://hg.mozilla.org/mozilla-central/rev/b0085234bc44
Description
•