1295157 - l10n: create script to migrate translations from DTD files to properties files

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Julian Descottes [:jdescottes]]

In the scope of devtools.html, all localized strings will be moved to *.properties files and loaded dynamically.

We should provide a script that can be run on L10N repositories that will take care of migrating the translations from *.dtd files to *.properties files.

Said script should be provided to localizers before merge day [1], and they should decide whether to use it or not.

[1] https://wiki.mozilla.org/L10n:Migration#Merge

Comment 1

[Julian Descottes [:jdescottes]]

I have a basic migration script working, but I'm still not sure which files will be available in the repository when the script will run. Francesco, could you give me some more details about the update process for the L10N repos? We can use the scenario below as a use case.

Let's say in release N we have:
- A.dtd, B.properties

In release N+1 we have 
- B.properties, C.properties

Let's say A.dtd contains S1 & S2. S1 was moved to B.properties, rename S1.new ; S2 was moved to C.properties, renamed S2.new. If we ignore the "migration" aspect, this means we have one deleted file, a new string in B.properties and a new file with a new string. How would that typically be handled?

A migration script should run when A.dtd is still available (so that it can get existing translations), but I guess this means the new files & strings will not be in the repository. So when migrating S1 from A.dtd to B.properties, the localization note for S1.new will not be available. I can still create S1.new with the current translation, but it would be without a localization note.

Is this fine? I could also include the localization notes in the migration script if really needed.

Comment 2

[Francesco Lodolo [:flod]]

First of all let's CC Axel, since he's the one actually taking care of merge day.

Let's ignore the migration part for a moment. Let's say we have one new string in B.properties, and another new one in C.properties. If localizers work directly on files, they will need to add them manually. If they use tools, they'll get the new strings from en-US and add them when they're localized.

If the migration script is part of the merge day operations, it will be run when the old file is still available (assuming it was translated at some point), before tools have a chance to remove anything.
https://wiki.mozilla.org/L10n:Migration#Merge

Localization notes: that's tricky, I'd say they should be moved. But we should also make sure to be able to exclude locales.

I'm sure Axel is understanding how you're parsing and writing these files, given his experience with such tools over the years (and how misleadingly easy it might look).

Comment 3

[Julian Descottes [:jdescottes]]

Thanks for the information Francesco!

Axel, do you have any guidelines I should use for writing this script? The script I have for now expects to have the old DTD files available and copies the current translations to their new location. As discussed with Francesco, I can include the new localization notes as well if need be.

Comment 4

[Axel Hecht [:Pike]]

We're trying to find out what guidelines and coding patterns to use ourselves right now in bug 1288141. Nothing tangible or tested in practice yet, sadly.

Having the original files around as source is what we'll be doing.

The IDs in the new file can probably be chosen to your liking, same IDs or new ones or un-postfixed ones. It's a new file, so no ID changes or something like that required.

For parsing, I'd recommend the code in python/compare-locales/compare_locales/parser.py.

We're going to think about what we want to do a bit next week, so in the week after next, I might have a more crisp idea what we intend to go with.

Comment 5

[Julian Descottes [:jdescottes]]

Thanks for feedback Axel!

I propose the following plan for the dtd->properties migration: wait until after the next merge date (sep 12) to start landing migration patches. This gives us Sep 12 -> Nov 07 (merge date for 53) to come up with a script for devtools dtd->properties migration. In the meantime, I expect we will have guidelines for writing & executing the migration script (keeping in mind this devtools migration is simpler and has a smaller scale than the whole l20n migration).

Also as I mentioned in Bug 1299070, the dtd->properties migration will most likely not be done at once for all modules in devtools. We are starting with the toolbox, the inspector and the webconsole, the other tools might only come later. If this is a big concern and you would prefer us to do a single migration, we can prioritize differently. 

Not sure if this matters, but if we stick to our current plan and migrate only inspector, webconsole and toolbox, I estimate the amount of strings to migrate to be a little over 100.   

For now I blocked all dtd->properties bugs on this one.

Comment 6

[Julian Descottes [:jdescottes]]

Can you have a look at comment 5 and let me know if you disagree / have concerns with anything here?

Comment 7

[Axel Hecht [:Pike]]

I think we should also think about the localizers that are actually working against central. So it's not that we're winning a full 6 weeks by just pushing forward 1.5 weeks. These are technically more savvy, so they should be able to run conversion scripts on their own, but we shouldn't make them have unlocalized Nightly devtools for a few weeks while they'd wait for the tools.

Comment 8

[Julian Descottes [:jdescottes]]

(In reply to Axel Hecht [:Pike] from comment #7)
> I think we should also think about the localizers that are actually working
> against central. So it's not that we're winning a full 6 weeks by just
> pushing forward 1.5 weeks. These are technically more savvy, so they should
> be able to run conversion scripts on their own, but we shouldn't make them
> have unlocalized Nightly devtools for a few weeks while they'd wait for the
> tools.

I see. I can't block devtools-html progress for too long though, so I will have to land anyway in the near future. 

For localizers working with central, how about I provide the script I have for the moment and make myself available for support? I can even run the script if it's not for too many locales. Again this concerns around ~100 strings, so not a huge migration.

For other localizers: if I understood correctly, I need to wait until you have a clearer picture of your migration plans, because this might impact the kind of script I have to write?

So, I would still wait for merge day before landing migration patches, with simple migration scripts and documentation for localizers working with central. And for the other localizers, this effectively gives us 6 more weeks to have a migration plan ready. Is this ok?

(I will clean up my current script and upload it here shortly)

Comment 9

[Axel Hecht [:Pike]]

Tarek offered some help on the migration code on the python side.

What I'm looking for is that we'll deal with the migrations on a progressive enhancment path.

The most vital part is to avoid that localizers need to redo localizations. Julien, can you share that code with Tarek?

For using the existing parsers, do

    from compare_locales.parser import getParser
    parser = getParser('somefile.dtd')  # only extension matters here
    parser.readFile('path/to/localized/dtd')
    entities, map = parser.parse()
    entity = entities[map['some_key']]
    value = entity.val

There's no serializer code in that code base, but .properties are simple enough, I guess.

For enhancements, for the general migrations, we're looking at features to persist local comments in the l10n files, and attribution from version control.
We're also looking at making the conversion data-driven in the sense that localization tools could update their internal database and maintain internal data, and also that the new migrations wouldn't generate code from scratch, but just data to generate the new file.

At large scale, we care about the attribution, but that shouldn't block the first migrations.

As the localization repos are under a different ldap permission, the best first steps to get the migrations run is probably to have localizers run them themselves. Running the on behalf of localizers would also be part of our incremental improvements.

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: bug1295157.v1.patch

@Tarek: Can you or someone from your team give me technical feedback on my current implementation? (still need to add unit tests though!) 

@Axel: here is a first version of the script based on our discussions on the topic so far. I would like to get your feedback on the requirements/specs for this script.

The script takes two arguments:
- the path to the target l10n repository
- the path to a configuration file (or folder)

The configuration file contains the list of properties that should be moved from dtds to properties. My current approach is to have one configuration file per migration bug. If the user provides a path to a configuration folder, all configuration files from this folder will be executed.

For each string to migrate, the script does the following:
- retrieve the current translation on the local l10n repository
- retrieve the localization notes on mozilla-central
- create a new entry in the target properties file

Exceptions/errors handled:
- if the localization note cannot be retrieved, an empty localization note is added instead
- if the string is already found in the target properties file, it is skipped

What this script does not do:
- does not interact with hg
- does not delete any dtd file after the migration

I still mainly work on this script using Github (https://github.com/juliandescottes/devtools-l10n-migration). I moved it in central here. I would still prefer to use github because this is a temporary script and is just for a few devtools modules, but up to you.

We will need to move forward with the migration bugs soon. I need to start landing the first migration bugs next week, otherwise this is going to block too much devtools-html work.

I hope we can iterate on the script fast enough so that it impacts localization teams as little as possible. If we don't manage to do so, we should keep in mind this is only about a few devtools modules (and the devtools audience is most likely used to see documentation and software in english).

Let me know what you think!

Comment 11

[Julian Descottes [:jdescottes]]

Quick update on the plan for resuming work on the dtd->properties migration bugs here. I am waiting until after merge day to start landing them. I expect the first patches to land in the beginning of next week.

Comment 12

[Axel Hecht [:Pike]]

Comment on attachment 8789598 [details] [diff] [review]
bug1295157.v1.patch

Review of attachment 8789598 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the lag, slowly getting back from travel and catching up on things.

I think the conversion format might paint us into a corner, it seems a bit backwards in ordering.

AFAICT, it also doesn't allow to create new comments, and only appends? Also, there doesn't seem to be a way to correct old unfortunate comments on the way?

For wholesome devtools, that might be OK, but for fx feature migrations, we'll likely need more.

::: python/devtools/migrate-l10n/migrate/conf/bug1294186
@@ +18,5 @@
> +styleinspector.dtd:browserStylesLabel = inspector.properties:inspector.browserStyles.label
> +styleinspector.dtd:filterStylesPlaceholder = inspector.properties:inspector.filterStyles.placeholder
> +styleinspector.dtd:addRuleButtonTooltip = inspector.properties:inspector.addRule.tooltip
> +styleinspector.dtd:togglePseudoClassPanel = inspector.properties:inspector.togglePseudo.tooltip
> +styleinspector.dtd:noPropertiesFound = inspector.properties:inspector.noProperties

This file looks as if it is the wrong way around, I think. Creating a dtd from a properties file. The code seems to do the right thing, but this only order only works as long as there's a 1-1 correspondence.

Also, I wonder if this needs some templating language? "Use this value plus that value" or something. Maybe not immediately.

I've glanced at the existing files for usage of entity references in the DTD, and found just one,

canvasdebugger.dtd:<!ENTITY canvasDebuggerUI.reloadNotice2   "the page to be able to debug &lt;canvas&gt; contexts.">

I guess that needs to expand entity references. Not sure if this hits us in existing localized files already.

That's also an example of strings glued together in DTDs. Not sure if you'll want to migrate them to something that looks like l20n. We use something we call "overlay" there, basically creating sanitized innerHTML fragments. Sadly that's completely under-doc'ed right now, best pointer we found was https://github.com/l20n/l20n.js/blob/master/examples/locales/example.en-US.ftl#L3-L4 as an example.

Comment 13

[Julian Descottes [:jdescottes]]

Thanks for the feedback!

(In reply to Axel Hecht [:Pike] from comment #12)
> Comment on attachment 8789598 [details] [diff] [review]
> bug1295157.v1.patch
> 
> Review of attachment 8789598 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the lag, slowly getting back from travel and catching up on things.
> 
> I think the conversion format might paint us into a corner, it seems a bit
> backwards in ordering.
> 
> AFAICT, it also doesn't allow to create new comments, and only appends?
> Also, there doesn't seem to be a way to correct old unfortunate comments on
> the way?
> 
> For wholesome devtools, that might be OK, but for fx feature migrations,
> we'll likely need more.
> 

That's correct, but I'm really only writing this script with the devtools string migrations in mind, and I don't mean this to be reused for other firefox string migrations. I am only doing dtd -> properties, whereas I think fx migrations will have to be properties/dtd -> ftl? I don't think these should be handled by the same script. 

The scope of this bug is to create a script for devtools.html string migrations. Which means only dtd->properties files, for some devtools modules.

> ::: python/devtools/migrate-l10n/migrate/conf/bug1294186
> @@ +18,5 @@
> > +styleinspector.dtd:browserStylesLabel = inspector.properties:inspector.browserStyles.label
> > +styleinspector.dtd:filterStylesPlaceholder = inspector.properties:inspector.filterStyles.placeholder
> > +styleinspector.dtd:addRuleButtonTooltip = inspector.properties:inspector.addRule.tooltip
> > +styleinspector.dtd:togglePseudoClassPanel = inspector.properties:inspector.togglePseudo.tooltip
> > +styleinspector.dtd:noPropertiesFound = inspector.properties:inspector.noProperties
> 
> This file looks as if it is the wrong way around, I think. Creating a dtd
> from a properties file. The code seems to do the right thing, but this only
> order only works as long as there's a 1-1 correspondence.
> 

The 1-1 correspondence is fine for devtools I think. I will change the order, agree it's confusing.

> Also, I wonder if this needs some templating language? "Use this value plus
> that value" or something. Maybe not immediately.
> 
> I've glanced at the existing files for usage of entity references in the
> DTD, and found just one,
> 
> canvasdebugger.dtd:<!ENTITY canvasDebuggerUI.reloadNotice2   "the page to be
> able to debug &lt;canvas&gt; contexts.">
> 
> I guess that needs to expand entity references. Not sure if this hits us in
> existing localized files already.
> 

Good point. I think it's safe to assume for devtools that only HTML entities might be found in localized strings. And if a non-HTML entity is found, then log an error and skip the string? FWIW, CanvasDebugger migration to HTML is not planned at the moment.

> That's also an example of strings glued together in DTDs. Not sure if you'll
> want to migrate them to something that looks like l20n. We use something we
> call "overlay" there, basically creating sanitized innerHTML fragments.
> Sadly that's completely under-doc'ed right now, best pointer we found was
> https://github.com/l20n/l20n.js/blob/master/examples/locales/example.en-US.
> ftl#L3-L4 as an example.

Comment 14

[Julian Descottes [:jdescottes]]

Attachment: bug1295157.v2.patch

Here is an updated patch applying the comments from Axel's review. 

Axel: can you confirm if you are interested in a script only for devtools dtd->properties migration? From your last comment I feel like you are expecting a script that could do both devtools & l20n (and that would probably be incompatible with devtools.html timeframe).

Comment 15

[Francesco Lodolo [:flod]]

Comment on attachment 8792182 [details] [diff] [review]
bug1295157.v2.patch

Review of attachment 8792182 [details] [diff] [review]:
-----------------------------------------------------------------

One possible issue: we might end up with files with mixed line endings.

::: python/devtools/migrate-l10n/migrate/main.py
@@ +37,5 @@
> +                'devtools/client/locales/en-US/')
> +
> +
> +# HTML parser to translate HTML entities in dtd files
> +HTML_PARSER = HTMLParser.HTMLParser()

Aren't you missing an import HTMLParser for this to work?

@@ +51,5 @@
> +    if prop_filename in central_prop_cache:
> +        return central_prop_cache[prop_filename]
> +
> +    url = DEV_BASE_URL + prop_filename
> +    logging.info('loading localization notes from central: {%s}' % url)

notes -> file

@@ +126,5 @@
> +        logging.warning('string already migrated, skipping: {%s}' % prop_name)
> +        return
> +    # Skip the string and log an error if an existing entry is found, but with
> +    # a different value.
> +    if ('\n' + prop_name + '=') in prop_file_content:

This is not safe: you're assuming the entry is defined as "name=value", you should check for prop_name\s*= instead
If locale has properties defined as "name = value", you'd end up with duplicates.

Comment 16

[Axel Hecht [:Pike]]

(In reply to Julian Descottes [:jdescottes] from comment #14)
> Created attachment 8792182 [details] [diff] [review]
> bug1295157.v2.patch
> 
> Here is an updated patch applying the comments from Axel's review. 
> 
> Axel: can you confirm if you are interested in a script only for devtools
> dtd->properties migration? From your last comment I feel like you are
> expecting a script that could do both devtools & l20n (and that would
> probably be incompatible with devtools.html timeframe).

Yeah, I think we want to limit the scope here to what devtools needs.

I'm obviously looking for inspirations and possible code-reuse for our wider tasks for l20n.

Comment 17

[Julian Descottes [:jdescottes]]

Attachment: bug1295157.v3.patch

Thanks for the feedback Francesco, applied your comments on the patch uploaded here.

Comment 18

[Julian Descottes [:jdescottes]]

Unblocking devtools-html bugs. 
We will most likely only land Bug 1294186 at the moment (i.e. 19 strings).

Comment 19

[Francesco Lodolo [:flod]]

I've used the script for bug 1294186, and it worked fine.

Reminder: if we plan to use this, we also need to remove DTD files, and add the new .properties

Comment 20

[Francesco Lodolo [:flod]]

Julian, can you clarify which tools are expected to migrate in this cycle?

If it's more than bug 1294186, I'd like to see this in m-c at some point, and start planning for its use on merge day.

Comment 21

[Julian Descottes [:jdescottes]]

(In reply to Francesco Lodolo [:flod] from comment #20)
> Julian, can you clarify which tools are expected to migrate in this cycle?
> 
> If it's more than bug 1294186, I'd like to see this in m-c at some point,
> and start planning for its use on merge day.

Thanks for giving the script a try!

In this cycle, only Bug 1294186 will land. Other devtools modules will be migrated in FF53 or later. Scope of the migration has been reduced quite a bit since we started discussing this topic, sorry about that.

Also, the last status is that we will not migrate other tools the way we did it for the inspector. The inspector had a direct conversion from XUL to HTML, which is why we had to reuse the existing strings. The other tools will be rewritten from scratch. 
Depending on the tool, the UI could be different from the XUL one and the strings will also be different (that's the case for the webconsole for instance).
 
Saddens me a bit to say that, but I don't think we should push it to mc right now. What do you think?

Comment 22

[Julian Descottes [:jdescottes]]

Attachment: bug1295157.v4.patch

Updated patch to rely on mozilla central URL to retrieve localization notes as the inspector migration bug has now landed.

Comment 23

[Francesco Lodolo [:flod]]

(In reply to Julian Descottes [:jdescottes] from comment #21)
> Saddens me a bit to say that, but I don't think we should push it to mc
> right now. What do you think?

If the scope is only inspector, I agree that there's not much gain from landing in m-c. It's good to have this around in a bug in case plans change.

Comment 24

[Julian Descottes [:jdescottes]]

(In reply to Francesco Lodolo [:flod] from comment #23)
> (In reply to Julian Descottes [:jdescottes] from comment #21)
> > Saddens me a bit to say that, but I don't think we should push it to mc
> > right now. What do you think?
> 
> If the scope is only inspector, I agree that there's not much gain from
> landing in m-c. It's good to have this around in a bug in case plans change.

It looks like the netmonitor will also be migrated in this cycle. That represents a 100 additional strings.
Francesco: should we resume our efforts with this script? From your point of view, is there something missing before we try to get it into mc?

Comment 25

[Francesco Lodolo [:flod]]

(In reply to Julian Descottes [:jdescottes] from comment #24)
> (In reply to Francesco Lodolo [:flod] from comment #23)
> It looks like the netmonitor will also be migrated in this cycle. That
> represents a 100 additional strings.
> Francesco: should we resume our efforts with this script? 

This sounds like a good idea.

> From your point of view, is there something missing before we try 
> to get it into mc?

There are a couple of pieces, but I don't think they should be part of your script. 
1. Mercurial transactions: "hg rm" old DTD files, "hg add" brand new .properties files, "hg commit" the changes with a bug number.
2. Locale exclusions: we don't want to touch locales already working on central, because we would create unnecessary conflicts for them.

Turning NI to pike to confirm this part, since he's the one actually running merge day.

Comment 26

[Axel Hecht [:Pike]]

(In reply to Francesco Lodolo [:flod] from comment #25)
> Turning NI to pike to confirm this part, since he's the one actually running
> merge day.

Agreed.

Comment 27

[Julian Descottes [:jdescottes]]

Attachment: Bug 1295157 - create devtools l10n migration script for dtd to properties migration;

For devtools-html, devtools will progressively stop using DTD files and will use only properties
files. To avoid retranslating already localized strings in every locale, this script can be used
by localization teams to automatically migrate strings moved in the scope of the devtools-html
project.

Review commit: https://reviewboard.mozilla.org/r/86348/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86348/

Comment 28

[Julian Descottes [:jdescottes]]

Comment on attachment 8801671 [details]
Bug 1295157 - create devtools l10n migration script for dtd to properties migration;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86348/diff/1-2/

Comment 29

[Julian Descottes [:jdescottes]]

Axel: I initially wanted to request review from :gps but since you will be the main user of the script I prefer to let you decide here. 

For information I'll be on PTO between Oct 20 and Nov 4. I'll still try to read bugmail and follow up on urgent issues, but expect some lag on my side.

Comment 30

[Axel Hecht [:Pike]]

Comment on attachment 8801671 [details]
Bug 1295157 - create devtools l10n migration script for dtd to properties migration;

https://reviewboard.mozilla.org/r/86348/#review85102

I've found a couple of not-so-edgecase problems, not sure if it's useful to do those inline.

Generally speaking, there needs to be a ton of error handling, both on a per-file basis, and on a per-entry basis. You should expect to find DTD files that are nothing but comments. I'd even prefer to not create the target file in those cases.

Anything from a missing file, to a file with nothing but comments, to a file with just a few strings, to all strings translated will need to be handled, and create equally partial translated results.

I also noticed that the file-wide comment in https://dxr.mozilla.org/mozilla-central/source/devtools/client/locales/en-US/font-inspector.properties wouldn't make it to the output?

I would also expect to cache the parsed results for the DTDs.

We made some good progress on the actual algorithm we intend to implement for l20n just last week, maybe that's helpful? https://github.com/mozilla-l10n/roadmap/issues/13

Sadly, that doesn't come with a single line of code yet.

Comment 31

[Francesco Lodolo [:flod]]

Looks like bug 1308500 (netmonitor) just landed.

Comment 32

[Julian Descottes [:jdescottes]]

Comment on attachment 8801671 [details]
Bug 1295157 - create devtools l10n migration script for dtd to properties migration;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86348/diff/2-3/

Comment 33

[Julian Descottes [:jdescottes]]

Comment on attachment 8801671 [details]
Bug 1295157 - create devtools l10n migration script for dtd to properties migration;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86348/diff/3-4/

Comment 34

[Julian Descottes [:jdescottes]]

(In reply to Francesco Lodolo [:flod] from comment #31)
> Looks like bug 1308500 (netmonitor) just landed.

Indeed, just added a migration config file for bug1308500 to the last patch attached here.

Comment 35

[Julian Descottes [:jdescottes]]

Comment on attachment 8801671 [details]
Bug 1295157 - create devtools l10n migration script for dtd to properties migration;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86348/diff/4-5/

Comment 36

[Julian Descottes [:jdescottes]]

(In reply to Axel Hecht [:Pike] from comment #30)
> Comment on attachment 8801671 [details]
> Bug 1295157 - create devtools l10n migration script for dtd to properties
> migration;
> 
> https://reviewboard.mozilla.org/r/86348/#review85102
> 
> I've found a couple of not-so-edgecase problems, not sure if it's useful to
> do those inline.
> 
> Generally speaking, there needs to be a ton of error handling, both on a
> per-file basis, and on a per-entry basis. You should expect to find DTD
> files that are nothing but comments. I'd even prefer to not create the
> target file in those cases.
> 
> Anything from a missing file, to a file with nothing but comments, to a file
> with just a few strings, to all strings translated will need to be handled,
> and create equally partial translated results.
> 
> I also noticed that the file-wide comment in
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/locales/en-US/
> font-inspector.properties wouldn't make it to the output?
> 
> I would also expect to cache the parsed results for the DTDs.
> 
> We made some good progress on the actual algorithm we intend to implement
> for l20n just last week, maybe that's helpful?
> https://github.com/mozilla-l10n/roadmap/issues/13
> 
> Sadly, that doesn't come with a single line of code yet.

Thanks for the review! I tried to address most of your comments here. 

A new file is now only created if we have found a valid translation to migrate. 

If the file can't be created for some reason we skip the current string.

If a new file is created, we try to get the header from mozilla central (tried reusing the properties parser for this, but it kept including the localization note for the first entity so I had to write my own). If the header can't be found, we fallback and add the default license header. [the reason I did not include file comments is that I don't know of any mechanism to update them in L10N repos in case they are updated in the root file. I thought it was better to keep the file in mc as the source of truth for those]

If a localization note can't be found the entry is still migrated but without any localization note. In an earlier version I was adding empty localization notes, but since we have entries in devtools which don't have any localization note, I think it makes more sense to simply have no localization note at all for them. 

Also now using a cache for the parsed entries from the DTD parser.

Comment 37

[Axel Hecht [:Pike]]

Comment on attachment 8801671 [details]
Bug 1295157 - create devtools l10n migration script for dtd to properties migration;

https://reviewboard.mozilla.org/r/86348/#review85888

I've got one more nit below, with that, r=me.

Thanks for the perseverance

::: python/devtools/migrate-l10n/migrate/main.py:78
(Diff revision 5)
> +        line = line.strip('\n').strip('\r')
> +
> +        if line.startswith('#'):
> +            # Comment line, add to the current comment buffer.
> +            comment_buffer.append(line)
> +        elif line.startswith(prop_name + '='):

flod had mentioned this in comment 15, this is very tied to the coding style, but that's more of a nit.

Comment 38

[Julian Descottes [:jdescottes]]

Comment on attachment 8801671 [details]
Bug 1295157 - create devtools l10n migration script for dtd to properties migration;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86348/diff/5-6/

Comment 39

[Julian Descottes [:jdescottes]]

Comment on attachment 8801671 [details]
Bug 1295157 - create devtools l10n migration script for dtd to properties migration;

https://reviewboard.mozilla.org/r/86348/#review85888

Thanks for the review!

> flod had mentioned this in comment 15, this is very tied to the coding style, but that's more of a nit.

Good catch, I modified it in one check but forgot the other :/

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/070e1a84f3e9
create devtools l10n migration script for dtd to properties migration;r=Pike

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/070e1a84f3e9

Comment 42

[Julian Descottes [:jdescottes]]

Marco: I can't remember why we decided to flip this one back to qe-verify+. The script has already been tried by Francesco and I don't think we need additional QA here (not to mention this is pretty different from what we usually send for QA verification). Would you be ok to set qe-verify- here?

Comment 43

[Marco Mucci [:MarcoM]]

(In reply to Julian Descottes [:jdescottes] from comment #42)
> Marco: I can't remember why we decided to flip this one back to qe-verify+.
> The script has already been tried by Francesco and I don't think we need
> additional QA here (not to mention this is pretty different from what we
> usually send for QA verification). Would you be ok to set qe-verify- here?

No problem Julian, I'll set it to qe-verify-.  Thanks.