Switch from test to lint job for checking fluent files

ASSIGNED
Assigned to

Status

()

enhancement
P3
normal
ASSIGNED
2 years ago
6 days ago

People

(Reporter: flod, Assigned: berning5)

Tracking

(Blocks 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

2 years ago
.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?
(Reporter)

Comment 2

2 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.
> 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.
Duplicate of this bug: 1441636

Comment 5

a year 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

4 months 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.
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
You need to log in before you can comment on or make changes to this bug.