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

Already on GitHub? Sign in to your account

Fixed #5849 -- Strip whitespace from blocktrans #1773

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Contributor

Bouke commented Oct 18, 2013

See ticket 5849 for background on this PR. The patch was written by mpessas, the PR rebased by me. This PR supersedes PR #803.

@aaugustin aaugustin and 1 other commented on an outdated diff Nov 2, 2013

django/utils/translation/__init__.py
@@ -194,3 +194,9 @@ def get_language_info(lang_code):
return LANG_INFO[generic_lang_code]
except KeyError:
raise KeyError("Unknown language code %s and %s." % (lang_code, generic_lang_code))
+
+
+trim_whitespace_re = re.compile('\s*\n\s*')
+def trim_whitespace(s):
+ """Trim the whitespace from s."""
@aaugustin

aaugustin Nov 2, 2013

Owner

That docstring doesn't add much info. It isn't useful to paraphrase a function's signature!

@Bouke

Bouke Nov 2, 2013

Contributor

Indeed!

@aaugustin aaugustin and 1 other commented on an outdated diff Nov 2, 2013

django/utils/translation/__init__.py
@@ -194,3 +194,9 @@ def get_language_info(lang_code):
return LANG_INFO[generic_lang_code]
except KeyError:
raise KeyError("Unknown language code %s and %s." % (lang_code, generic_lang_code))
+
+
+trim_whitespace_re = re.compile('\s*\n\s*')
+def trim_whitespace(s):
+ """Trim the whitespace from s."""
+ return trim_whitespace_re.sub(' ', s.strip('\n'))
@aaugustin

aaugustin Nov 2, 2013

Owner

Why not just s.strip()?

@Bouke

Bouke Nov 2, 2013

Contributor

You're right, makes no difference.

@aaugustin aaugustin and 1 other commented on an outdated diff Nov 2, 2013

django/utils/translation/trans_real.py
@@ -530,19 +531,22 @@ def templatize(src, origin=None):
pluralmatch = plural_re.match(t.contents)
if endbmatch:
if inplural:
+ singular = trim_whitespace(''.join(singular))
@aaugustin

aaugustin Nov 2, 2013

Owner

Isn't this going to trim whitespace regardless of whether the trans block we're dealing with has the trimmed option set?

@Bouke

Bouke Nov 2, 2013

Contributor

Good catch! The whole Parser/Lexer is unknown to me, so correct me if I've fixed it incorrectly. I've also added a unit test which showed that the trimmed option wasn't used in this case.

@aaugustin aaugustin commented on an outdated diff Nov 2, 2013

docs/topics/i18n/translation.txt
@@ -672,7 +672,32 @@ markers<contextual-markers>` using the ``context`` keyword:
.. code-block:: html+django
- {% blocktrans with name=user.username context "greeting" %}Hi {{ name }}{% endblocktrans %}
+ {% blocktrans with name=user.username context "greeting" %}Hi {{ name }}{%
+ endblocktrans %}
@aaugustin

aaugustin Nov 2, 2013

Owner

You cannot have a line break within a Django template tag.

@aaugustin aaugustin commented on an outdated diff Nov 2, 2013

docs/topics/i18n/translation.txt
+separate them. This is quite useful for indenting the content of a ``{%
+blocktrans %}`` tag without having the indentation characters end up in the
+corresponding entry in the PO file, which makes the translation process easier.
+
+For instance, the following ``{% blocktrans %}`` tag::
+
+ {% blocktrans trimmed %}
+ First sentence.
+ Second paragraph.
+ {% endblocktrans %}
+
+will result in the entry "First sentence. Second paragraph." in the PO file,
+compared to "\\nFirst sentence.\\nSecond sentence.\\n", if the ``trimmed``
+option had not been specified.
+
+.. versionchanged:: 1.7
@aaugustin

aaugustin Nov 2, 2013

Owner

Could you add a line in the release notes for 1.7 too?

Owner

aaugustin commented Nov 2, 2013

Overall this is pretty good; my comments are rather minor.

Once you've addressed them, feel free to mark the corresponding ticket as RFC yourself.

Fixed #5849 -- Strip whitespace from blocktrans
Add the trimmed option to the blocktrans tag to trim any newlines and
whitespace from its content.

This allows the developer to indent the blocktrans tag without adding
new lines and whitespace to the msgid in the PO file.

Thanks to mpessas for the initial patch and Dmitri Fedortchenko for the
report.
Contributor

Bouke commented Nov 2, 2013

Thanks for the review; I've updated the PR with the appropriate changes.

Contributor

Bouke commented Nov 8, 2013

Merged in 7a7c789

@Bouke Bouke closed this Nov 8, 2013

@Bouke Bouke deleted the Bouke:tickets/5849 branch Nov 8, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment