Closed Bug 1165906 Opened 5 years ago Closed 3 years ago

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

Categories

(Toolkit Graveyard :: Build Config, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Pike, Assigned: Pike)

References

Details

(Whiteboard: [gecko-l20n])

Attachments

(2 files, 1 obsolete file)

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)
Attached file MozReview Request: bz://1165906/Pike (obsolete) —
/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/
Attachment #8607532 - Flags: review?(gps)
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.
Attachment #8607532 - Flags: review?(gps) → feedback?(gps)
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'?
(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.
(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.
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.
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'
(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.
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?
Flags: needinfo?(l10n)
Attachment #8607532 - Flags: feedback?(gps)
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.
Assignee: nobody → l10n
Flags: needinfo?(l10n) → needinfo?(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.
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.
Attachment #8613540 - Flags: review?(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.
Attachment #8613540 - Flags: review?(gps) → review+
(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.
Flags: needinfo?(gps)
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 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.
Attachment #8613540 - Attachment description: MozReview Request: bug 1165906, add docs for l10n to the tree, r?gps → MozReview Request: bug 1165906, add docs for l10n to the tree, r=gps
Attachment #8613540 - Flags: review+
Attachment #8613540 - Flags: review+
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 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.
Attachment #8613540 - Flags: checkin?
... also flagging to keep this bug open for the actual work.
Keywords: leave-open
Attachment #8613540 - Flags: checkin? → checkin+
Attachment #8607532 - Attachment is obsolete: true
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
Blocks: gecko-l20n
Blocks: 1280906
Mass change dependency tree for bug 1279002 into a whiteboard keyword.
No longer blocks: gecko-l20n
Whiteboard: [gecko-l20n]
Depends on: 1303027
Depends on: 1303028
See Also: → 1361037
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.
Blocks: 1370176
No longer blocks: 1280906
Status: NEW → RESOLVED
Closed: 3 years ago
No longer depends on: 1303027, 1303028
Keywords: leave-open
Resolution: --- → FIXED
Summary: Unify l10n.ini and filter.py into a single (python-independent) format → Document l10n.ini and filter.py in gecko.rtd
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.