Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug 1193860: escape quotes in some DTD files #326

Merged
merged 1 commit into from Jan 14, 2016

Conversation

mathjazz
Copy link
Collaborator

Some DTD files require quotes to be escaped. This is my attempt to do so.

@jotes or @Osmose r?

The curly quotes (‘ and “) don't get escaped for some reason. Any idea why? I'll add tests.

@@ -78,6 +85,9 @@ def __init__(self, parser, path, source_resource=None):
self.source_resource = source_resource
self.entities = OrderedDict() # Preserve entity order.

# Bug 1193860: unescape quotes in some files
self.escape_quotes_on = 'mobile/android/base' in path and parser is DTDParser
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any better heuristic for this? Is it bad if we escape all quotes in all DTD files?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that was my original idea (do it for all .dtd files), but it's sadly not possible:
https://dxr.mozilla.org/mozilla-central/source/python/compare-locales/compare_locales/checks.py#389

We have to do it the ugly way. One day we should get rid of the hardcoded path and move this to project config.

@Osmose
Copy link
Contributor

Osmose commented Jan 13, 2016

r+, nice work! I think you should really avoid the keying on the translation file path, and I suspect you could do simpler than a regex, but I trust your judgement on whether to actually do those things.

@mathjazz
Copy link
Collaborator Author

Thanks. Turns out we don't need to escape curly quotes. I'll add tests tomorrow and then merge.

@gion-andri
Copy link

Maybe I misunderstand your patch or how DTDs work, in this case just ignore this comment.

But if I understand correct your patch, you always escape the quotes in the DTD files with '. I think, in DTD files, witch use a single quote to delimit the String (like search_strings.dtd), this does not work. I that case, the quote has to be replaced with something like '.

Background:
This commit:
http://hg.mozilla.org/releases/l10n/mozilla-aurora/rm/diff/6ac304b05f25/mobile/android/base/search_strings.dtd
caused these compare-locales errors:
https://l10n.mozilla.org/dashboard/compare?run=563338
This commit fixed the issue:
http://hg.mozilla.org/releases/l10n/mozilla-aurora/rm/diff/d7a76071741e/mobile/android/base/search_strings.dtd
Or maybe this is just a compare-locales issue.

@mathjazz
Copy link
Collaborator Author

Good point @gion-andri!

Could you please check if it looks better now?

@gion-andri
Copy link

This looks way better, thanks!

@jotes
Copy link
Collaborator

jotes commented Jan 14, 2016

r+

On Thu, Jan 14, 2016 at 9:04 AM, Gion-Andri Cantieni <
notifications@github.com> wrote:

This looks way better, thanks!


Reply to this email directly or view it on GitHub
#326 (comment).

Kind regards

​Jarek "jotes" Śmiejczak,
​P​
ython programmer​ by day, mozillian by heart..
Homepage: http://joyfulhack.in, Github: http://github.com/jotes
Mobile: +48693027040, Mozillian: http://mozillians.org/u/jotes

mathjazz added a commit that referenced this pull request Jan 14, 2016
Fix bug 1193860: escape quotes in some DTD files
@mathjazz mathjazz merged commit ef2925b into mozilla:master Jan 14, 2016
@mathjazz mathjazz deleted the bug-1193860 branch January 14, 2016 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants