1165906 - Document l10n.ini and filter.py in gecko.rtd

Status: RESOLVED FIXED

(Toolkit Graveyard :: Build Config, defect)

Description

[Axel Hecht [:Pike]]

l10n.ini and filter.py are overly complex, and they're only accessible to tools written in python.

Just .ini files aren't great, but I came across .toml, https://github.com/toml-lang/toml, which is also used by Rust/Cargo, which seems like a good fit.

In a first stab, I'll try to get the existing files converted, get feedback on that, and then in a second step, actually implement reading those files from compare-locales.

I also expect to make these files host information like:
- what's a devtools strings, or "techie"
- when to use android's error checker
- when to not use properties error checker (hi calendar)

Comment 1

[Axel Hecht [:Pike]]

Attachment: MozReview Request: bz://1165906/Pike

/r/9041 - bug 1165906, create l10n.toml files to aggregate l10n.ini and filter.py

Pull down this commit:

hg pull -r 827fba5d00e3476b7d5d5e3c41fd9056934578a2 https://reviewboard-hg.mozilla.org/gecko/

Comment 2

[Axel Hecht [:Pike]]

Comment on attachment 8607532 [details]
MozReview Request: bz://1165906/Pike

Actually, this is much more of a feedback request.

Gregory, starting off with you, probably gonna post to the newsgroup, too.

Feel free to loop in other folks at your descrition.

Re reviewboard, is there a trick to make copies look more incremental? 'cause the toml files are copies of the ini files, plus markup fixes, which are easier to look at.

Comment 3

[Francesco Lodolo [:flod]]

Nit that I think is not relevant at this stage: you're referencing a webapprt/locales/l10n.toml that's not created in this patch.

> path = '^searchplugins/.*$'

Why moving away from 'searchplugins\/.+\.xml'?

Comment 4

[Axel Hecht [:Pike]]

(In reply to Francesco Lodolo [:flod] (UTC+2) from comment #3)
> Nit that I think is not relevant at this stage: you're referencing a
> webapprt/locales/l10n.toml that's not created in this patch.

oops, right.

> > path = '^searchplugins/.*$'
> 
> Why moving away from 'searchplugins\/.+\.xml'?

'cause we fall back for list.txt now, too.

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to Axel Hecht [:Pike] from comment #0)
> l10n.ini and filter.py are overly complex, and they're only accessible to
> tools written in python.
> 
> Just .ini files aren't great, but I came across .toml,
> https://github.com/toml-lang/toml, which is also used by Rust/Cargo, which
> seems like a good fit.

Drive-by: why is this information not encoded in moz.build?

I'd also add that TOML is a really special snowflake: 600K Google hits; Yaml has 2.5M; JSON has 75M.  I would want some argument for why we'd use TOML to express logic rather than writing nice code.

Comment 6

[Axel Hecht [:Pike]]

moz.build is python code, and we're working on infrastructure written in javascript to get l10n contributions.

I think we can do encode stuff regarding l10n in a way that doesn't require to execute python, and toml is a way that doesn't come with a host of complexity. Notably, I found it more expressive than yaml, and have better language support. And json just sucks comment-wise.

Comment 7

[Gregory Szorc [:gps]]

I agree with nalexander's comments here. As pretty as TOML looks, I'd rather not introduce yet another markup language.

I'm not buying your argument that Python is a deal breaker here. What exactly is the use case that prevents us from using Python? Python is already a requirement for pretty much any developer tool in the Firefox universe. There are some niches that try to do lots of things in JS, even through Node. But I think there's just too much Python to avoid. And when you consider the tight coupling with the Firefox build system and automation, Python becomes my default choice for things like this.

With support for sub-contexts in moz.build files, you can do some really expressive things. e.g.

  with L10nFilter:
      MODULE = 'browser'
      PATH = 'defines.inc'
      KEY = 'MOZ_LANGPACK_CONTRIBUTORS'
      ACTION = 'ignore'

Comment 8

[Axel Hecht [:Pike]]

(In reply to Gregory Szorc [:gps] from comment #7)
> I agree with nalexander's comments here. As pretty as TOML looks, I'd rather
> not introduce yet another markup language.
> 
> I'm not buying your argument that Python is a deal breaker here. What
> exactly is the use case that prevents us from using Python? Python is
> already a requirement for pretty much any developer tool in the Firefox
> universe. There are some niches that try to do lots of things in JS, even
> through Node. But I think there's just too much Python to avoid. And when
> you consider the tight coupling with the Firefox build system and
> automation, Python becomes my default choice for things like this.

There's a bunch of existing and upcoming tooling around l10n where python doesn't play a role, and in particular anything with builds is in the way.

https://transvision.mozfr.org/ is mostly php
https://l10n.mozilla.org/ doesn't do builds, aka, any data for which I need an object dir is out of question
Translation editors that work on the web need hints from the data inside the browser, and that for sure can't execute python.
gaia tries to avoid python like the plague, IIRC, for good or bad reasons.

Firefox and builds is just one customer of this data, and its close ties to python right now has been in the way of tool development.

Comment 9

[Gregory Szorc [:gps]]

OK. You've convinced me why Python isn't ideal for you. That's fine.

But I'm still not sold on a) TOML b) whether this is the right solution. Let's focus on the latter since TOML is an implementation detail and thus a bikeshed.

The code as is is taking invisible-to-this-bug .ini and .py files and combining them into a somewhat complicated TOML file. From the code I see, it's impossible to tell if this is better or worse than the existing solution. You're asking me to take a giant leap of faith that what you've implemented will somehow work itself into a desirable end state. I hate saying "I need to see more before I can approve this" because that feels like stop energy. But, I need to see more before I can approve this.

This work appears to start a significant refactoring of l10n foo. I would appreciate something a little more than a patch that tells a partial story before I put a stamp of approval on this work.

Is that a reasonable request?

Comment 10

[Axel Hecht [:Pike]]

Attachment: with copies to show relationship with .ini

Yes, that's fair.

FWIW, I think my change triggered a bug in the headless repo logic. Attaching my local `hg show tip` for reference, I don't see the copies show up on reviewboard nor on https://reviewboard-hg.mozilla.org/gecko/rev/827fba5d00e3476b7d5d5e3c41fd9056934578a2. Doesn't make the patch more readable, though, at least on hg serve without side-by-side diffs.

I'm trying to wrap my head around how to give the right context for this change. I guess there could be two ways:

One, show the impact of the change by code samples in code using it.
Second, document the impact in documentation, that'd be used to actually write code.

Which probably starts with "explain what we're doing now", which somehow makes me think that that's "document the thing". I wish I could document what I'd like to have, but I guess that's still hard to grok without documentation of the status quo?

Really depends on what kind of context makes it into your brain more easily.

PS: I understand that having such docs show up on https://gecko.readthedocs.org/en/latest/ won't hurt to begin with.

Comment 11

[Axel Hecht [:Pike]]

PS: pushed to http://hg.mozilla.org/users/axel_mozilla.com/gecko/rev/827fba5d00e3, too, if that helps.

Comment 12

[Gregory Szorc [:gps]]

I think documenting the existing l10n foo in the in-tree Sphinx docs (gecko.readthedocs.org) would be an excellent use of anybody's time. Far too few people understand how that all works. And documentation explaining the consumers and processes involved would be very helpful with assessing the direction of l10n.

Comment 13

[Axel Hecht [:Pike]]

Attachment: MozReview Request: bug 1165906, add docs for l10n to the tree, r=gps

bug 1165906, add docs for l10n to the tree, r?gps

Document the role of l10n.ini, filter.py, file paths and checks.

Also add a glossary for the jargon used in the doc.

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8613540 [details]
MozReview Request: bug 1165906, add docs for l10n to the tree, r=gps

https://reviewboard.mozilla.org/r/9041/#review8773

Aside from rst nits, this is excellent documentation and helps l10n comprehension immensely. As long as you don't break `mach build-docs`, I think it is fine to land docs improvements like these with minimal to no review. Docs > no docs > wrong docs.

::: python/compare-locales/docs/index.rst:14
(Diff revision 2)
> +idea to check the :doc:`glossary` for what localization is, and which terms

Since the namespace for these docs isn't limited to l10n foo, you'll want to use a less generic name than "glossary."

::: python/compare-locales/docs/index.rst:117
(Diff revision 2)
> +For any missing file, this function is called with :py:data:`mod` being
> +the *module*, and :py:data:`path` being the relative path inside 

Not sure you need `py:data:` here. ``mod`` and ``path`` should suffice methinks.

Comment 15

[Gregory Szorc [:gps]]

(In reply to Axel Hecht [:Pike] from comment #10)
> Created attachment 8612285 [details]
> with copies to show relationship with .ini
> 
> Yes, that's fair.
> 
> FWIW, I think my change triggered a bug in the headless repo logic.
> Attaching my local `hg show tip` for reference, I don't see the copies show
> up on reviewboard nor on
> https://reviewboard-hg.mozilla.org/gecko/rev/
> 827fba5d00e3476b7d5d5e3c41fd9056934578a2. Doesn't make the patch more
> readable, though, at least on hg serve without side-by-side diffs.

I pulled the changeset and copy info is there. I guess the style we have installed on reviewboard-hg isn't showing copy info. Also, current versions of Review Board don't deal with copy info very well. This is fixed upstream and we're awaiting the next release to upgrade.

> I'm trying to wrap my head around how to give the right context for this
> change. I guess there could be two ways:
> 
> One, show the impact of the change by code samples in code using it.
> Second, document the impact in documentation, that'd be used to actually
> write code.
> 
> Which probably starts with "explain what we're doing now", which somehow
> makes me think that that's "document the thing". I wish I could document
> what I'd like to have, but I guess that's still hard to grok without
> documentation of the status quo?
> 
> Really depends on what kind of context makes it into your brain more easily.
> 
> PS: I understand that having such docs show up on
> https://gecko.readthedocs.org/en/latest/ won't hurt to begin with.

I think having docs of what we do now (what I just reviewed) is excellent and very beneficial to reviewing any change.

If you start from the docs you've written and you construct additional documentation (possibly as commit messages) that describe exactly what you are doing, the intended end state, and why, I think I can grok enough about what's happening to grant review.

Comment 16

[Francesco Lodolo [:flod]]

https://reviewboard.mozilla.org/r/9041/#review8783

::: python/compare-locales/moz.build:8
(Diff revision 2)
> +    BUG_COMPONENT = ('Localization Tools and Tools', 'compare-locales')

I think "Localization Infrastructure and Tools" here.

::: python/compare-locales/docs/index.rst:86
(Diff revision 2)
> +    Used from javascript and C++. When used from js, also comes with 

JavaScript

::: python/compare-locales/docs/index.rst:142
(Diff revision 2)
> +the localized files in the merge dir first, the the localized file, and then

the -> then

::: python/compare-locales/docs/index.rst:180
(Diff revision 2)
> +Now that we talked about in-depth how to expose content to localizers,

in-depth about (swap order)

::: python/compare-locales/docs/index.rst:183
(Diff revision 2)
> +We host a mercurial repository per locale and per branch. Most of our

Mercurial

(went grammarnazi on the doc)

Comment 17

[Axel Hecht [:Pike]]

Comment on attachment 8613540 [details]
MozReview Request: bug 1165906, add docs for l10n to the tree, r=gps

bug 1165906, add docs for l10n to the tree, r=gps

Document the role of l10n.ini, filter.py, file paths and checks.

Also add a glossary for the jargon used in the doc.

Comment 18

[Axel Hecht [:Pike]]

Comment on attachment 8613540 [details]
MozReview Request: bug 1165906, add docs for l10n to the tree, r=gps

https://reviewboard.mozilla.org/r/9041/#review8813

Ship It!

Comment 19

[Axel Hecht [:Pike]]

Comment on attachment 8613540 [details]
MozReview Request: bug 1165906, add docs for l10n to the tree, r=gps

Fixed the comments and carrying forward the review, flagging this one for landing.

Comment 20

[Axel Hecht [:Pike]]

... also flagging to keep this bug open for the actual work.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/915d4854e649

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/915d4854e649

Comment 24

[Axel Hecht [:Pike]]

As part of how we intend to redo firefox l10n, getting a configuration that's easier to digest is gonna be important. Adding this to our tracker.

Drive by comment on our in-tree usage of yaml (for taskcluster): The usage of PyStache on top of yaml (*) does imply that using yaml-as-in-the-tree would come with significant additional dependencies for non-tree users, and yaml-as-yaml isn't semantically the exact same thing. That might sound pedantic, but I've re-used .properties enough to have war stories of sharing file formats with different semantics.

(*) http://gecko.readthedocs.io/en/latest/taskcluster/taskcluster/yaml-templates.html

Comment 25

[Axel Hecht [:Pike]]

Mass change dependency tree for bug 1279002 into a whiteboard keyword.

Comment 26

[Axel Hecht [:Pike]]

mozreview doesn't like me continuing the work here. Resolving this bug and reshaping it as documentation bug.

New work in its clone, bug 1370176.