622846 - [compare-locales] Add check for Android's princess handling of apostrophes

Status: RESOLVED FIXED

(Mozilla Localizations :: Infrastructure, defect)

Description

[Axel Hecht [:Pike]]

Android's XML string files are really just containers for something else, the funny rules are (hopefully complete?) at http://developer.android.com/guide/topics/resources/string-resource.html#FormattingAndStyling.

Let's add a check for that.

That check needs to be specific to just the DTD files in embedding/android, or in other words, it shouldn't run on any good DTD file, really. So let's reduce its scope to embedding/android and keep our fingers crossed that that stuff doesn't leak outside of there.

So I need to do two things, make the checks lookup pass the file so I can test the module, and write the check, and tests for it. Gonna be two changesets.

The original bug where this broke first is bug 622429.

Comment 1

[Axel Hecht [:Pike]]

Attachment: make selecting checks depend on the whole file obj, not just the leaf path

Comment 2

[Axel Hecht [:Pike]]

Attachment: actual android tests, just for embedding/android dtd files, with a good chunk of tests

Comment 3

[Axel Hecht [:Pike]]

I'd like to get feedback from the mobile folks on at least two parts in attachment 501340 [details] [diff] [review]:

- I'm running android DTD checks for all DTD files in embedding/android. Is that too much or too little or just right? We shouldn't run them on non-android DTDs, thus that choice.

- Are the tests OK or am I missing some other fatal thing? I check for unescaped ' and ", with and without the complete string in quotes, and for broken unicode escapes.

Comment 4

[Chris Cooper [:coop] (he/him)]

Comment on attachment 501339 [details] [diff] [review]
make selecting checks depend on the whole file obj, not just the leaf path

> class DTDChecker(Checker):
>-    '''Tests to run on DTD files.
>+    """Tests to run on DTD files.

Any particular reason for the syntax change here (and elsewhere)?
 
>     def setUp(self):
>-        p = getParser(self.filename)
>+        p = getParser(self.file.file)

Hrmm, self.file.file irritates me slightly (vs. say, self.file.name), but not enough to block review.

Comment 5

[Axel Hecht [:Pike]]

The .file is coming from the File class, changing that would be a pretty substantial rename. It's a tad confusing though, yeah.

The comment quote changes are just to make my emacs python mode happy, the syntax highlighting is thrown off by '''I'm stupid and anything after the apos is code'''.

Comment 6

[Chris Cooper [:coop] (he/him)]

Comment on attachment 501340 [details] [diff] [review]
actual android tests, just for embedding/android dtd files, with a good chunk of tests

>+class PrincessAndroid(Checker):
>+    """Checker for the string values that Android puts into an XML container.
>+
>+    http://developer.android.com/guide/topics/resources/string-resource.html#FormattingAndStyling
>+    has more info. Check for unescaped apostrophes and bad unicode escapes.
>+    """

LOL. I'm so glad it's got a nice comment so I don't have to ask you to rename it. ;)

Comment 7

[Axel Hecht [:Pike]]

Comment on attachment 501340 [details] [diff] [review]
actual android tests, just for embedding/android dtd files, with a good chunk of tests

Doh, I need to r- me here.

The quote stuff isn't working at all, as we're not dealing with the original strings, but DTD entries filling in the strings. That is, not
<string>"foo'bar"</string>
but
<!ENTITY .... well, how do I type "'"? Right, xml ents
<!ENTITY "&quot;foo&apos;bar&quot;">

That implies that I need to get the parsed text instead of the unparsed entity value for this check.

The right way to do this is to get the xml parser to create that value, which means that I need to refactor stuff a good deal.

Comment 8

[Axel Hecht [:Pike]]

http://hg.mozilla.org/l10n/compare-locales/rev/03a3db86c408, FIXED.