2
\$\begingroup\$

From time to time I translate some Android apps: the developer sends me an XML file, e.g.

<string name="foo">Foo</string>
<string name="baz">Baz</string>

and I send him back an XML file where the content of each element has been translated, e.g.

<string name="foo">Translated foo</string>
<string name="baz">Translated baz</string>

The problem arises when the developer adds some new text element and sends me a new file to be translated, e.g.

<string name="foo">Foo</string>
<string name="bar">Bar</string>
<string name="baz">Baz</string>

Thus I want to merge the two files, adding a marker at the beginning of the new lines to be translated. Given the previous example, this would mean generating a file like

<string name="foo">Translated foo</string>
<string name="bar">***new***Bar</string>
<string name="baz">Translated baz</string>

Since I didn't find any pre-existing solution I decided to write a small Python script myself which so far seems to do the job:

"""Usage: merge_strings.py [-m <mrk>] [-o <file> [-i]] <old_xml> <new_xml>

Substitutes the content of any 'string' tag from <new_xml> with the
content of a 'string' tag from <old_xml> with the same 'name' attribute,
if present, otherwise prepends it with <mrk>.
By default the result is printed to stdout.

Note: This program assumes that no two 'string' tags in the same file
have the same 'name' attribute. Furthermore, 'string' tags with names
unique to <old_xml> are ignored.

Options:
    -h --help                 Show this screen.
    -m <mrk> --marker <mrk>   Marker for new strings [default: ***new***].
    -o <file>                 Print to <file> instead.
    -i --interactive          Check before overwriting <file>.
"""

from os.path import isfile
from sys import exit

from docopt import docopt
import lxml.etree as etree


def merge_strings(old, new, marker):
    """
    Merge in place synonymous strings from 'old' into 'new'.
    Ignores strings unique to 'old' and prepends strings unique to
    'new' with 'marker'.
    """
    for s in new.iterfind('//string'):
        name = s.attrib['name']
        t = old.find("//string[@name='" + name + "']")

        if t is not None:
            s.text = t.text
        else:
            s.text = marker + s.text

def check_overwrite(path):
    """
    Check if we want to overwrite 'path' and exit if not.
    Defaults to no.
    """
    print("About to overwrite:", path)
    choice = input("Continue? [y/N]")

    if choice.lower() != 'y':
        exit(0)

def print_to_file(tree, path, interactive=False):
    if interactive and isfile(path):
        check_overwrite(path)

    with open(path, mode='wb') as f:
        tree.write(f, pretty_print=True,
                      encoding='utf-8',
                      xml_declaration=True)

def print_to_stdout(tree):
    print(etree.tostring(tree, pretty_print=True,
                               encoding='utf-8',
                               xml_declaration=True).decode('utf-8'))


if __name__ == '__main__':
    args = docopt(__doc__)

    old_tree = etree.parse(args['<old_xml>'])
    new_tree = etree.parse(args['<new_xml>'])

    merge_strings(old_tree, new_tree, args['--marker'])

    if args['-o']:
        print_to_file(new_tree, args['-o'], args['--interactive'])
    else:
        print_to_stdout(new_tree)

Here is the obligatory example output:

$cat tests/old.xml 
<?xml version="1.0" encoding="utf-8"?>
<resources>
  <string name="foo">Translated foo</string>
  <string name="baz">Translated baz</string>
</resources>

$cat tests/new.xml 
<?xml version="1.0" encoding="utf-8"?>
<resources>
  <string name="foo">Foo</string>
  <string name="bar">Bar</string>
  <string name="baz">Baz</string>
</resources>

$python merge_strings.py old.xml new.xml                                                   
<?xml version='1.0' encoding='utf-8'?>
<resources>
  <string name="foo">Translated foo</string>
  <string name="bar">***new***Bar</string>
  <string name="baz">Translated baz</string>
</resources>

Remarks: This is only my second Python program and it is the first time I dabble with XML. As such, I would like to know if this code follows Python best practices and if it can be improved in terms of readability and/or performance.

\$\endgroup\$
  • \$\begingroup\$ a better solution here is patches or a version control system IMO \$\endgroup\$ – Azsgy Oct 2 '15 at 21:34
  • \$\begingroup\$ @Atsch Those are good suggestions. Alas, those are out of my control, too. On the plus side, this problem makes for a nice little exercise. :) \$\endgroup\$ – A.P. Oct 2 '15 at 21:42
  • 1
    \$\begingroup\$ Even though I'm all for a programmtic solution try this one on for size: Keep a copy of the previous original file, add do a diff -p between previous and new file in the original language. This will give you a nice list of new texts to translate into your new file... \$\endgroup\$ – holroy Oct 2 '15 at 23:51
2
\$\begingroup\$

First a little disclaimer: I haven't worked with either docopt or etree before. So I can't comment on the usage/correctness of this

Style reviews

Overall the code looks OK, but there are stuff comment upon:

  • Process arguments at start – Instead of referring to args[whatever] throughout your main code part, I would check the different parameters at the start of your main block, and then set proper flags and variables to be used later on. This would also allow for proper handling if the arguments are missing were you could substitute some of them with default values
  • No handling of -m (or -i) argument? – Your usage strings says that you could use either -m or --marker, and that it has a default of ***new***. Can't see the code providing this? (This would be clearer if you had implemented previous point)
  • Not too keen on your naming – The names old, new, s and t are somewhat confusing. Maybe something along the lines of old_translation, new_source, native (or source?) and translation would be better?
  • Decide on one of ' and " – You have a bit of both... (Just like I often do... ;-) )
  • Avoid string concatenation – Instead of "//string[@name='" + name + "']", use "//string[@name='{}']".format(name). Note that I'm personally aiming for using single quotes in most strings, but if my new string is to have single quotes, you either need to escape this \' or switch around and use double quotes for that string
  • No error handling – There is little or none error handling if any of the file operations fails, or arguments missing, and so on
  • Mostly good docstrings – Even though they are lacking for the latter methods, and the style guidelines says to include a summary in the first line in multi-line docstrings
  • Recommendation is two newlines before def – If you increase to two newlines, the methods will stand out a little more, and you fill follow the guidelines even more.

Performance issue

One option I believe could hurt you performance wise is the search in old for any translated resource. This will all the time do an \$O(n)\$ search, and thusly render your entire code for merging to be \$O(n^2)\$.

If you instead had made a dictionary of all the old strings, the search could be more like \$O(1)\$ (or some low cost for checking the dictionary) at cost of a prerun through old. This would render the merge process to be \$2\cdot O(n)\$, which is similar/counted as \$O(n)\$. Where \$n\$ is the number of strings to be translated.

In other words, if using a dictionary for the old strings, you would most likely get a much faster merge operation when the number of strings to be translated increases.

PS! As it is a little late in the night in my part of the world, I have not written any new code. Please tell if some of the comments are not understandable or you don't know how to correct it

\$\endgroup\$
  • \$\begingroup\$ Thanks for your remarks. The arguments are entirely handled by docopt after parsing the main docstring (it's almost magic). As a disclaimer, I learned Haskell before Python, which is why I tend to find short variable names easier to work with. I will try to implement your suggestions as soon as I'll be at my main computer. In the meantime, could you explain in more detail how I could implement some better error handling? \$\endgroup\$ – A.P. Oct 3 '15 at 12:19
  • \$\begingroup\$ @A.P., gotta look into that docopt module, someday. Hopefully that covers error cases it self, but maybe it raises errors if the filename is missing (or similar required behaviour)? Outside of argument handling, there are three cases which you at least should check: 1) reading of old file, 2) reading of new file, and 3) writing result file. All these file operation are error prone as files could not exist, not be read-/writeable, and other stuff. Try encapsulating these within try ... except blocks, similar to what I've down in this answer \$\endgroup\$ – holroy Oct 3 '15 at 18:17
1
\$\begingroup\$

I have two minor style/readability notes.

I think saying "defaults to no" for check_overwrite(path) is a little misleading. Python functions can have default values that get used if no value is supplied for particular parameters, so I misunderstood at first and thought you had removed a parameter that used to be there. Instead you could be clear that it only accepts 'y' and exits for all other input.

It's also more idiomatic to use if var is None than is not None when you can so I'd flip the test in merge_strings.

    if t is None:
        s.text = marker + s.text
    else:
        s.text = t.text

You could use the ternary operator here. Opinion varies on whether it's good form but it would cut down to one line:

    s.text = marker + s.text if t is None else t.text
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.