Closed Bug 1525869 Opened 3 years ago Closed 2 years ago

Switch browser_misused_characters_in_strings.js to use fluent-syntax

Categories

(Firefox :: General, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 75
Tracking Status
firefox67 --- wontfix
firefox75 --- fixed

People

(Reporter: flod, Assigned: zbraniecki)

References

Details

Attachments

(1 file)

Bug 1517496 added checks for FTL files, but we should rely on the fluent-syntax package as soon as it's available in tree, instead of using the run-time parser.

Giving this a priority to take it out of the triage list - adjust as necessary.

Priority: -- → P3
Depends on: 1526875
Depends on: 1539192

FluentSyntax.jsm is in the tree now and it provides a Visitor class which helps run logic on the AST nodes: https://searchfox.org/mozilla-central/source/intl/l10n/FluentSyntax.jsm#1887. It's inspired by the Python ast modules NodeVisitor: https://docs.python.org/2/library/ast.html#ast.NodeVisitor. It's meant to be subclassed, and the subclass can add methods like visitTextElement(node).

I think the plan is more around using compare-locales for this? We already have the l10n linter in place, so it's a matter of adding checks, and I have the feeling Axel already has a POC somewhere (given he filed bug 1588176 a while ago).

Ah, great. The linter is a much more scalable solution to this, I agree. I wouldn't mind removing FluentSyntax.jsm, if it's not used anywhere else.

As Zibi mentioned on Slack, bug 1560038 will make FluentResource opaque with no way to inspect the runtime AST, which is why I came to this bug and commented about the Visitor. But moving the test to the linter will obviously be a solution, too.

Can you confirm that you're intending to replace that with c-l? If so, is there a bug for that? I'd like to wontfix this in such case.

Flags: needinfo?(l10n)

I've got a POC for c-l locally, but there's no bug, nor is there a timeline.

Flags: needinfo?(l10n)

I'm making progress on remaining loose ends in bug 1560038. Axel, do you have any ETA for this?

Flags: needinfo?(l10n)

I don't have an ETA, I've put that branch on the backburner after filing bug 1588176.

Flags: needinfo?(l10n)

Since Axel doesn't have any ETA for his work, and this is going to become a blocker for fluent-rs switch, can we discuss options to move forward?

Stas landed FluentSyntax.jsm and suggested switching to it. With fluent-rs landing we also get access to fluent-syntax crate which would allow us to do this in Rust.

How important is this test? Do we need to keep this as a blocker to bug 1560038?

Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(francesco.lodolo)

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #9)

How important is this test? Do we need to keep this as a blocker to bug 1560038?

The test was added for a reason (consistency), and it's important. I'd like to avoid going back to manually comment every single patch, and risk missing some in the process. It's also not great code, but it does what it's supposed to do (most of the time).

Having said that, I'm not the one who can (or should) answer about the blocking status, or how to prioritize work to fix it.

Flags: needinfo?(francesco.lodolo)

We should replace the test with a linter, and we should avoid losing coverage. It seems there was a linter in bug 1588176. What's the status of that work / why hasn't that landed?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(l10n)

It's a POC for, say, half the work. I can find chars as part of a linter, and I can exclude some parts of HTML syntax that like real apostrophes. I haven't spent time on what I should do with this outside of Firefox and outside of en-US.

Flags: needinfo?(l10n)

(In reply to Axel Hecht [:Pike] from comment #12)

It's a POC for, say, half the work. I can find chars as part of a linter, and I can exclude some parts of HTML syntax that like real apostrophes. I haven't spent time on what I should do with this outside of Firefox and outside of en-US.

I would like to land a polished version of this into the fluent linter on m-c, to unblock this bug and unblock bug 1560038. Is there any reason not to do so? If not, can you put up what you have so we can find someone to drive this over the line, on the assumption that you don't have time to do so yourself?

Flags: needinfo?(l10n)

I've looked at my code, and I hate it. I pushed it up on https://phabricator.services.mozilla.com/D61809 and explained why. Not sure when I'd have cycles to actually do that work myself, but I'm happy to see someone else tackle it, now that I know why I'd r- it ;-) .

Flags: needinfo?(l10n)
Assignee: nobody → gandalf
Status: NEW → ASSIGNED

this bug blocks bug 1560038 and it doesn't seem like there's any better solution coming out soon, so I wrote a patch that migrates us to use FluentSyntax.jsm in a fairly plain way.

I'm basically composing every Pattern to skip any non-text-elements and combine the TextElements into a single string that is then tested just as before.

Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/29241bff5209
Use FluentSyntax for misused characters in strings test. r=stas
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 75
You need to log in before you can comment on or make changes to this bug.