Fixed #29713 -- Added a check to warn if LANGUAGE_CODE does not use the standard language id format #10340

darvid7 · 2018-08-27T04:11:19Z

This PR adds a check to warn if the LANGUAGE_CODE in settings.py does not use the standard language id format. Ticket 29713

included test cases
updated docs

LilyFoote

This is a good start. There's a few things I think can be improved.

LilyFoote · 2018-08-27T06:23:42Z

django/core/checks/translation.py

+    match_result = re.match(LANGUAGE_CODE_PATTERN, settings.LANGUAGE_CODE)
+    errors = []
+    if not match_result:
+        errors.append(Warning(


I think I'd make this an Error - I don't think translation works at all if this is wrong.

LilyFoote · 2018-08-27T06:25:40Z

tests/check_framework/test_translation.py

+
+    @override_settings(ROOT_URLCONF='check_framework.translation.valid_language_code_format_ll_only')
+    def test_valid_language_code_format_ll_only(self):
+        settings.LANGUAGE_CODE = "en"


Does it work if you use override_settings for this too? That would be cleaner.

Yes test work fine without it.

LilyFoote · 2018-08-27T06:26:49Z

tests/check_framework/test_translation.py

+    @override_settings(ROOT_URLCONF='check_framework.translation.invalid_language_code_format_ll_only')
+    def test_invalid_language_code_format_ll_only(self):
+        invalid_codes = ["EN", "eN"]
+        for code in invalid_codes:


I'm not sure there's much value in testing two invalid codes instead of just one.

Fair point, test one value in TranslationCheckTests. Added TranslationLanguageCodeRegexTest to test regex using subTest()

LilyFoote · 2018-08-27T06:27:51Z

tests/check_framework/test_translation.py

+
+class TranslationCheckTests(SimpleTestCase):
+
+    @override_settings(ROOT_URLCONF='check_framework.translation.valid_language_code_format_ll_only')


Why do you override ROOT_URLCONF?

Copied from other files, removed it.

LilyFoote

There's a few more improvements that can still be made.

LilyFoote · 2018-08-27T07:01:01Z

tests/check_framework/test_translation.py

+                matches = re.match(LANGUAGE_CODE_PATTERN, lang_code)
+                self.assertEqual(matches, None)
+
+    def test_invalid_language_code_format_ll_cc(self):


You can combine these tests.

LilyFoote · 2018-08-27T07:01:15Z

tests/check_framework/test_translation.py

+        for lang_code in invalid_codes:
+            with self.subTest(lang_code=lang_code):
+                matches = re.match(LANGUAGE_CODE_PATTERN, lang_code)
+                self.assertEqual(matches, None)


Prefer self.assertIsNone(matches)

LilyFoote · 2018-08-27T07:04:30Z

tests/check_framework/test_translation.py

+class TranslationCheckTests(SimpleTestCase):
+
+    def test_valid_language_code_format_ll_only(self):
+        settings.LANGUAGE_CODE = "en"


We should use override_settings for this, to ensure we clean up after ourselves once the test is complete, avoiding breaking unrelated tests.

LilyFoote

Just a few nitpicks left. Looks good to me otherwise.

LilyFoote · 2018-08-27T07:20:45Z

django/core/checks/translation.py

+@register(Tags.translation)
+def check_setting_language_code(app_configs, **kwargs):
+    """
+    Warn if language code is in the wrong format. Language codes are generally represented in lower-case with a dash


The word generally doesn't add any value in my opinion.

I'm not certain Warn is the best word now that this is an Error, but I'm not sure how to phrase it instead.

Errors if language code is in the wrong format?

darvid7 · 2018-08-28T00:07:12Z

@Ian-Foote Please take another look.
I've updated the regex to use language_code_re in utils.translations.trans_real.py that I think checks for a more comprehensive set of LANGUAGE_CODEs.

LilyFoote

Still looks good to me.

carltongibson

OK. Great. Thanks.

(Smallest of niggles... Ta!)

carltongibson · 2018-09-03T08:03:24Z

django/core/checks/__init__.py

@@ -13,6 +13,7 @@
 import django.core.checks.security.sessions  # NOQA isort:skip
 import django.core.checks.templates  # NOQA isort:skip
 import django.core.checks.urls  # NOQA isort:skip
+import django.core.checks.translation  # NOQA isort:skip


Can you move this above ...checks.urls please?

(Presumably we're skipping isort here to stop it putting these imports amongst the other actual imports but they're alphabetical within that.)

carltongibson · 2018-09-03T08:03:57Z

django/core/checks/registry.py

@@ -16,6 +16,7 @@ class Tags:
    signals = 'signals'
    templates = 'templates'
    urls = 'urls'
+    translation = 'translation'


Again, I think keep alphabetical please.

standard language id format

carltongibson · 2018-09-03T08:44:29Z

Super stuff. Thanks @darvid7! Welcome aboard. 🎉

darvid7 · 2018-09-03T09:20:23Z

Thank you ^^

timgraham · 2018-09-03T22:51:40Z

django/core/checks/translation.py

+@register(Tags.translation)
+def check_setting_language_code(app_configs, **kwargs):
+    """
+    Errors if language code is in the wrong format. Language codes specification outlined by


Please wrap docstrings at 79 characters. The second sentence is missing a period.

timgraham · 2018-09-03T22:52:14Z

django/core/checks/translation.py

+    errors = []
+    if not match_result:
+        errors.append(Error(
+            "LANGUAGE_CODE in settings.py is {}. It should be in the form ll or ll-cc where ll is the language and cc "


Please wrap these lines at 79 characters.

timgraham · 2018-09-03T22:52:16Z

django/core/checks/translation.py

+    Errors if language code is in the wrong format. Language codes specification outlined by
+    https://en.wikipedia.org/wiki/IETF_language_tag#Syntax_of_language_tags
+    """
+    match_result = re.match(language_code_re, settings.LANGUAGE_CODE)


Not sure an intermediate variable is needed.

timgraham · 2018-09-03T22:52:22Z

django/core/checks/translation.py

+            "is the country. Examples include: it, de-at, es, pt-br. The full set of language codes specifications is "
+            "outlined by https://en.wikipedia.org/wiki/IETF_language_tag#Syntax_of_language_tags".format(
+                settings.LANGUAGE_CODE),
+            id="translation.E001",


Use single quotes (unless a string contains a single quote) as described in Python coding style.

timgraham · 2018-09-03T22:53:26Z

django/core/checks/translation.py

+            "LANGUAGE_CODE in settings.py is {}. It should be in the form ll or ll-cc where ll is the language and cc "
+            "is the country. Examples include: it, de-at, es, pt-br. The full set of language codes specifications is "
+            "outlined by https://en.wikipedia.org/wiki/IETF_language_tag#Syntax_of_language_tags".format(
+                settings.LANGUAGE_CODE),


) goes on the next line.

timgraham · 2018-09-03T22:55:58Z

docs/ref/checks.txt

+
+The following checks are performed on your translation configuration:
+
+* **translation.E001**: LANGUAGE_CODE in settings.py is ``<language_code>``.


The :setting:`LANGUAGE_CODE` setting...

(not all projects use settings.py)

timgraham · 2018-09-03T22:56:08Z

docs/ref/checks.txt

+   country. Examples include: ``it``, ``de-at``, ``es``, ``pt-br``. The full
+   set of language codes specifications is outlined by
+   https://en.wikipedia.org/wiki/IETF_language_tag#Syntax_of_language_tags
+


Use a single blank line between sections.

timgraham · 2018-09-03T22:56:28Z

tests/check_framework/test_translation.py

+
+class TranslationCheckTests(SimpleTestCase):
+
+    @override_settings(LANGUAGE_CODE="eu")


Use single quotes.

timgraham · 2018-09-03T22:57:36Z

tests/check_framework/test_translation.py

+    @override_settings(LANGUAGE_CODE="eu")
+    def test_valid_language_code_format_ll_only(self):
+        result = check_setting_language_code(None)
+        self.assertEqual(len(result), 0)


I'd go with assertEqual(check_setting_language_code(None), []). That's a bit easier to debug in the case of a failure since the error will be visible as opposed to just, e.g. 1 != 0.

timgraham · 2018-09-03T23:00:28Z

tests/check_framework/test_translation.py

+        result = check_setting_language_code(None)
+        self.assertEqual(len(result), 1)
+        error = result[0]
+        self.assertEqual(error.id, 'translation.E001')


I'm thinking it might be better to use the approach in test_caches.py rather than repeating the error message here twice.

ngnpope · 2018-09-04T09:10:24Z

@timgraham - only just seen your review here.

I worked on a follow up to this yesterday as I noticed many of the same issues highlighted in your review, but only just opened it this morning: #10367. Have updated it to incorporate some of the changes you requested that I missed.

darvid7 · 2018-09-04T09:25:00Z

@timgraham @pope1ni thanks for your feedback, should I make another PR to address the changes or should I leave it up to #10367

ngnpope · 2018-09-04T09:37:48Z

Hi David. I'd leave it up to #10367 as I believe everything is addressed there.

darvid7 force-pushed the pycon branch 8 times, most recently from ccfa84b to ffc605f Compare August 27, 2018 04:41

LilyFoote suggested changes Aug 27, 2018

View reviewed changes

darvid7 force-pushed the pycon branch 4 times, most recently from 0512d64 to d25cf53 Compare August 27, 2018 06:57

LilyFoote suggested changes Aug 27, 2018

View reviewed changes

darvid7 force-pushed the pycon branch from d25cf53 to 65b6a1d Compare August 27, 2018 07:13

LilyFoote approved these changes Aug 27, 2018

View reviewed changes

darvid7 force-pushed the pycon branch 4 times, most recently from 7f2c2ee to 0fefa15 Compare August 28, 2018 00:04

LilyFoote approved these changes Aug 28, 2018

View reviewed changes

carltongibson approved these changes Sep 3, 2018

View reviewed changes

Fixed #29713 -- Added a check to warn if LANGUAGE_CODE does not use the

292f73f

standard language id format

darvid7 force-pushed the pycon branch from 0fefa15 to 292f73f Compare September 3, 2018 08:21

carltongibson merged commit 5db8d61 into django:master Sep 3, 2018

timgraham reviewed Sep 3, 2018

View reviewed changes

ngnpope mentioned this pull request Sep 4, 2018

Refs #29713 -- Improved error message from translation system check. #10367

Merged


		class TranslationCheckTests(SimpleTestCase):

		@override_settings(ROOT_URLCONF='check_framework.translation.valid_language_code_format_ll_only')


		The following checks are performed on your translation configuration:

		* translation.E001: LANGUAGE_CODE in settings.py is ``<language_code>``.


		class TranslationCheckTests(SimpleTestCase):

		@override_settings(LANGUAGE_CODE="eu")

Fixed #29713 -- Added a check to warn if LANGUAGE_CODE does not use the standard language id format #10340

Fixed #29713 -- Added a check to warn if LANGUAGE_CODE does not use the standard language id format #10340

Conversation

darvid7 commented Aug 27, 2018 • edited

LilyFoote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LilyFoote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LilyFoote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darvid7 commented Aug 28, 2018

LilyFoote left a comment

Choose a reason for hiding this comment

carltongibson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carltongibson commented Sep 3, 2018

darvid7 commented Sep 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngnpope commented Sep 4, 2018

darvid7 commented Sep 4, 2018

ngnpope commented Sep 4, 2018

darvid7 commented Aug 27, 2018 •

edited