Allowing FORMAT_MODULE_PATH setting to be list of paths. #1236

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Contributor

mbrochh commented Jun 1, 2013

This patch includes tests for the iter_format_modules method (which was never
tested before). It also includes a test to ensure backwards compatibility and
to test the new functionality.

The patch also includes changes to thos parts of the docs that mention
the FORMAT_MODULE_PATH setting.

@mbrochh mbrochh commented on an outdated diff Jun 2, 2013

django/utils/formats.py
@@ -45,7 +45,12 @@ def iter_format_modules(lang):
if check_for_language(lang):
@mbrochh

mbrochh Jun 2, 2013

Contributor

This file contains the actual new feature. Before, FORMAT_MODULE_PATH was supposed to be a string. Now, it can be either a string or a list of strings.

All the rest, the reversing of the list, the loading of the modules, stays untouched.

@mbrochh mbrochh commented on the diff Jun 2, 2013

docs/ref/settings.txt
@@ -1187,6 +1187,18 @@ like::
__init__.py
@mbrochh

mbrochh Jun 2, 2013

Contributor

I described the new way to use this setting in the docs.

@mbrochh mbrochh commented on an outdated diff Jun 2, 2013

docs/topics/i18n/formatting.txt
@@ -157,7 +157,10 @@ To use custom formats, specify the path where you'll place format files first.
To do that, just set your :setting:`FORMAT_MODULE_PATH` setting to the package
@mbrochh

mbrochh Jun 2, 2013

Contributor

In this part of the docs I changed the example to reflect the new way of using this setting, just to raise awareness that this is possible.

@mbrochh mbrochh commented on an outdated diff Jun 2, 2013

tests/utils_tests/test_formats.py
+ sys.meta_path.pop(0)
+
+ def _get_generator_result(self, generator):
+ """
+ Helper method to turn the generator into a list.
+
+ We need this because we want to make assertions based on the returned
+ list.
+
+ """
+ result = []
+ for item in generator:
+ result.append(item)
+ return result
+
+ def test_returns_correct_default(self):
@mbrochh

mbrochh Jun 2, 2013

Contributor

I created this test simply to increase test coverage. Testing the default behaviour here.

@mbrochh mbrochh commented on an outdated diff Jun 2, 2013

tests/utils_tests/test_module/formats/__init__.py
@@ -0,0 +1,4 @@
+"""
@mbrochh

mbrochh Jun 2, 2013

Contributor

We already had a folder test_module in utils_tests which was used to test module loading behaviour in another test file. I thought this would be a good place to place my formats folder (and it's subfolders) in order to test module loading for the iter_format_modules method as well. I put this docstring here in case someone wonders what these files are good for.

@apollo13 apollo13 and 2 others commented on an outdated diff Jun 18, 2013

docs/ref/settings.txt
@@ -1187,6 +1187,18 @@ like::
__init__.py
formats.py
+You can also set this setting to a list of Python paths, for example::
+
+ FORMAT_MODULE_PATH = [
+ 'some_app.formats',
+ 'mysite.formats',
+ ]
+
+Please note that the order of the list will get reversed. When Django searches
+for a certain format, it will go through all given Python paths until it finds
+a module that actually defines the given format. This means, the packages
+further down in the list would override the ones further up.
@apollo13

apollo13 Jun 18, 2013

Owner

That goes against any behavior from TEMPLATE_DIRS, INSTALLED_APPS, STATICFILES_DIRS -- so don't reverse them.

@mbrochh

mbrochh Jun 18, 2013

Contributor

I agree. Will change that ASAP.

@timgraham

timgraham Sep 18, 2013

Owner

@mbrochh do you think you'll have a chance to make these changes or should we close this PR for now?

@mbrochh

mbrochh Sep 18, 2013

Contributor

@timgraham really sorry for the delay. Got swamped with work and then started travelling. I would still want to finish this pull request. I will work on it next week, the change should actually be quite simple.

Owner

apollo13 commented Jun 18, 2013

Where is an accepted ticket for this pull request?

Contributor

mbrochh commented Oct 24, 2013

@apollo13 I rebased this branch in my fork to the latest commit of Django. Then I force pushed this branch into my fork. I think because of this, some comments of you got lost.

You had asked me to change the behaviour so that the values in the FORMAT_MODULE_PATH settings are not reversed, which is done now. Sorry again for the huge delay.

@timgraham timgraham and 1 other commented on an outdated diff Oct 24, 2013

django/utils/formats.py
@@ -45,10 +45,15 @@ def iter_format_modules(lang, format_module_path=None):
Does the heavy lifting of finding format modules.
"""
if check_for_language(lang):
- format_locations = ['django.conf.locale.%s']
- if format_module_path:
- format_locations.append(format_module_path + '.%s')
- format_locations.reverse()
+ format_locations = []
+ if settings.FORMAT_MODULE_PATH:
+ if isinstance(settings.FORMAT_MODULE_PATH, basestring):
@timgraham

timgraham Oct 24, 2013

Owner

basestring isn't Python 3 compatible. Please use six.text_type instead.

@mbrochh

mbrochh May 18, 2014

Contributor

That doesn't work. According to this documentation, six.string_types is what I need.

@timgraham timgraham commented on an outdated diff Oct 24, 2013

docs/ref/settings.txt
@@ -1242,6 +1242,18 @@ like::
__init__.py
formats.py
+You can also set this setting to a list of Python paths, for example::
+
+ FORMAT_MODULE_PATH = [
+ 'mysite.formats',
+ 'some_app.formats',
+ ]
+
+When Django searches for a certain format, it will go through all given Python
+paths until it finds a module that actually defines the given format. This
+means, the packages further up in the list would override the ones further
@timgraham

timgraham Oct 24, 2013

Owner

"This means that" (remove comma)
further -> farther (use farther when referring to physical distance)

@timgraham timgraham and 1 other commented on an outdated diff Oct 24, 2013

docs/topics/i18n/formatting.txt
@@ -157,7 +157,10 @@ To use custom formats, specify the path where you'll place format files first.
To do that, just set your :setting:`FORMAT_MODULE_PATH` setting to the package
where format files will exist, for instance::
- FORMAT_MODULE_PATH = 'mysite.formats'
+ FORMAT_MODULE_PATH = [
+ 'some_app.formats'
@timgraham

timgraham Oct 24, 2013

Owner

missing comma at of end of line

would help to include a short .. versionchanged:: 1.7 here as well

@mbrochh

mbrochh Oct 24, 2013

Contributor

@timgraham should I put only this code snippet into the versionchanged block, or also lines 157ff or should I only put a note "Note that as of version 1.7 this setting can be either a string or a list of strings" into the versionchanged block?

@timgraham

timgraham Oct 24, 2013

Owner

I'd just go with the note.

@timgraham timgraham and 1 other commented on an outdated diff Oct 24, 2013

tests/utils_tests/test_formats.py
+
+ def setUp(self):
+ # This is a hack to make sure that we can import modules found in the
+ # tests/ folder.
+ sys.meta_path.insert(0, ProxyFinder())
+
+ def tearDown(self):
+ sys.meta_path.pop(0)
+
+ def _get_generator_result(self, generator):
+ """
+ Helper method to turn the generator into a list.
+
+ We need this because we want to make assertions based on the returned
+ list.
+
@timgraham

timgraham Oct 24, 2013

Owner

I'd omit the newline at the end of each docstring

@mbrochh

mbrochh May 18, 2014

Contributor

I will change that, but I'd like to point out that this newline is recommended in PEP8 and it does look a lot better when it is there, but obviusly there is no point in changing the whole Django codebase to that PEP8 standard, now (although, I'd volunteer to do that, if anyone else sees any value in that).

@timgraham timgraham and 1 other commented on an outdated diff Oct 24, 2013

tests/utils_tests/test_formats.py
+from .test_module_loading import ProxyFinder
+
+
+class IterFormatModulesTestCase(TestCase):
+ """Tests for the ``iter_format_modules`` method."""
+ longMessage = True
+
+ def setUp(self):
+ # This is a hack to make sure that we can import modules found in the
+ # tests/ folder.
+ sys.meta_path.insert(0, ProxyFinder())
+
+ def tearDown(self):
+ sys.meta_path.pop(0)
+
+ def _get_generator_result(self, generator):
@timgraham

timgraham Oct 24, 2013

Owner

this function is probably unnecessary -- you should just be able to cast the generator to a list in order to evaluate it, e.g. list(iter_format_modules('en'))

@mbrochh

mbrochh May 18, 2014

Contributor

When I try to cast that generator into a list, I get this error: *** Error in argument: "(iter_format_modules('en'))"

@timgraham

timgraham May 18, 2014

Owner

hm, I didn't have a problem: see http://dpaste.com/2398P7X/

@mbrochh

mbrochh May 19, 2014

Contributor

I swear to god, when I tried that yesterday, it crashed, not it works. o_O

Owner

timgraham commented Oct 24, 2013

The new functionality should also be mentioned in the release notes (docs/releases/1.7.txt)

Owner

timgraham commented Mar 1, 2014

The feature freeze for 1.7 is next Thursday, any chance of addressing my last set of comments before then?

Contributor

mbrochh commented May 16, 2014

oh dear, forgot about this one. I will rebase to 1.7 and submit a new patch that addresses the review comments...

Owner

timgraham commented May 16, 2014

Maybe you made a typo, but actually it needs to be rebased on master (1.8) as 1.7 is feature frozen now.

Contributor

mbrochh commented May 16, 2014

right, that's what I meant.
On 17 May 2014 07:07, "Tim Graham" notifications@github.com wrote:

Maybe you made a typo, but actually it needs to be rebased on master (1.8)
as 1.7 is feature frozen now.


Reply to this email directly or view it on GitHubhttps://github.com/django/django/pull/1236#issuecomment-43388516
.

Contributor

mbrochh commented May 18, 2014

@timgraham here we go. I rebased the current master into my patch and applied the codereview hints that you provided. Sorry that due to the rebase parts of our discussion are now lost, again.

@mbrochh mbrochh and 1 other commented on an outdated diff May 18, 2014

django/utils/formats.py
@@ -47,10 +48,15 @@ def iter_format_modules(lang, format_module_path=None):
Does the heavy lifting of finding format modules.
"""
if check_for_language(lang):
- format_locations = ['django.conf.locale.%s']
- if format_module_path:
- format_locations.append(format_module_path + '.%s')
- format_locations.reverse()
+ format_locations = []
+ if settings.FORMAT_MODULE_PATH:
+ if isinstance(settings.FORMAT_MODULE_PATH, six.string_types):
@mbrochh

mbrochh May 18, 2014

Contributor

@timgraham you suggested the use of six.text_types here but that didn't work. This one correctly returns True if the setting is a string (even if it is a unicode string).

@timgraham

timgraham May 18, 2014

Owner

Looks good, my mistake.

@timgraham timgraham and 1 other commented on an outdated diff May 18, 2014

django/utils/formats.py
@@ -3,6 +3,7 @@
import decimal
import datetime
from importlib import import_module
+import six
@timgraham

timgraham May 18, 2014

Owner

This should be Django's vendored copy from django.utils import six

@mbrochh

mbrochh May 19, 2014

Contributor

Oh god, I'm an idiot. It's even imported already further down...

@timgraham timgraham commented on an outdated diff May 18, 2014

django/utils/formats.py
@@ -47,10 +48,15 @@ def iter_format_modules(lang, format_module_path=None):
Does the heavy lifting of finding format modules.
"""
if check_for_language(lang):
- format_locations = ['django.conf.locale.%s']
- if format_module_path:
- format_locations.append(format_module_path + '.%s')
- format_locations.reverse()
+ format_locations = []
+ if settings.FORMAT_MODULE_PATH:
+ if isinstance(settings.FORMAT_MODULE_PATH, six.string_types):
+ format_module_path_setting = [settings.FORMAT_MODULE_PATH, ]
@timgraham

timgraham May 18, 2014

Owner

don't need the trailing comma

@timgraham timgraham commented on an outdated diff May 18, 2014

docs/ref/settings.txt
@@ -1378,6 +1378,19 @@ like::
__init__.py
formats.py
+As of version 1.8 you can also set this setting to a list of Python paths,
@timgraham

timgraham May 18, 2014

Owner

Use a .. versionchanged:: 1.8 annotation instead of saying "As of version 1.8"

@timgraham timgraham and 1 other commented on an outdated diff May 18, 2014

docs/ref/settings.txt
@@ -1378,6 +1378,19 @@ like::
__init__.py
formats.py
+As of version 1.8 you can also set this setting to a list of Python paths,
+for example::
+
+ FORMAT_MODULE_PATH = [
+ 'mysite.formats',
+ 'some_app.formats',
+ ]
+
+When Django searches for a certain format, it will go through all given
+Python paths until it finds a module that actually defines the given
+format. This means that the packages farther up in the list would override
@timgraham

timgraham May 18, 2014

Owner

I think "take precedence" would be a better word choice than "override"

@mbrochh

mbrochh May 19, 2014

Contributor

I think both terms should be clear. However, I have improved the note a bit to make sure that it is not the whole file that overrides the one below but really just format that overrides the same format in a file farther down (and I fixed another farther - I hate to be a non native speaker, hehe).

New text:

  When Django searches for a certain format, it will go through all given
    Python paths until it finds a module that actually defines the given
    format. This means that formats defined in packages farther up in the list
    would take precendence over the same formats in packages farther down.

@timgraham timgraham commented on an outdated diff May 18, 2014

docs/releases/1.8.txt
@@ -133,7 +133,10 @@ Forms
Internationalization
^^^^^^^^^^^^^^^^^^^^
-* ...
+* :setting:`FORMAT_MODULE_PATH` can now be a list of strings representing
+ module paths. This allows to import several format modules from different
@timgraham

timgraham May 18, 2014

Owner

"to import" -> "importing"

@timgraham timgraham commented on an outdated diff May 18, 2014

docs/releases/1.8.txt
@@ -133,7 +133,10 @@ Forms
Internationalization
^^^^^^^^^^^^^^^^^^^^
-* ...
+* :setting:`FORMAT_MODULE_PATH` can now be a list of strings representing
+ module paths. This allows to import several format modules from different
+ reusable apps. It also allows to override those custom formats in your main
@timgraham

timgraham May 18, 2014

Owner

"to override" -> "overriding"

@timgraham timgraham commented on an outdated diff May 18, 2014

docs/topics/i18n/formatting.txt
- FORMAT_MODULE_PATH = 'mysite.formats'
+.. versionchanged:: 1.8
+
+ Note that as of version 1.8 this setting can be either a string or a list
@timgraham

timgraham May 18, 2014

Owner

This note should be something like "The ability to specify FORMAT_MODULE_PATH as a list was added. Previously, only a single string value was supported."

@timgraham timgraham and 1 other commented on an outdated diff May 18, 2014

tests/utils_tests/test_formats.py
@@ -0,0 +1,86 @@
+"""Tests for ``django/utils/formats.py``."""
+import sys
+
+from django.test import TestCase
+from django.utils.formats import iter_format_modules
+
+from .test_module_loading import ProxyFinder
+
+
+class IterFormatModulesTestCase(TestCase):
+ """Tests for the ``iter_format_modules`` method."""
+ longMessage = True
+
+ def setUp(self):
@timgraham

timgraham May 18, 2014

Owner

Is this setUp/tearDown stuff needed? I commented it out and the tests still passed.

@mbrochh

mbrochh May 19, 2014

Contributor

You are right. I'm sure when I started this patch a year ago, this was needed, but now it seems to work. Will remove this.

Owner

timgraham commented May 18, 2014

This is looking good. If you can address my latest round of comments, please go ahead and squash your commits and follow our commit message guidelines. I'll commit it shortly thereafter. Thanks!

Contributor

mbrochh commented May 19, 2014

@timgraham I have added your recent comments and have squashed the commits. Hope my commit message is good enough :)

Owner

timgraham commented May 19, 2014

I fixed a couple flake8 warnings and merged in 950b6de, thanks.

timgraham closed this May 19, 2014

Owner

timgraham commented May 19, 2014

Actually, there is a failing test so I reverted this for now:

======================================================================
FAIL: test_iter_format_modules (i18n.tests.FormattingTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tim/code/django/tests/i18n/tests.py", line 748, in test_iter_format_modules
    self.assertEqual(list(iter_format_modules('de', 'i18n.other.locale')), [test_de_format_mod, de_format_mod])
AssertionError: Lists differ: [<module 'django.conf.locale.d... != [<module 'i18n.other.locale.de...

First differing element 0:
<module 'django.conf.locale.de.formats' from '/home/tim/code/django/django/conf/locale/de/formats.pyc'>
<module 'i18n.other.locale.de.formats' from '/home/tim/code/django/tests/i18n/other/locale/de/formats.pyc'>

Second list contains 1 additional elements.
First extra element 1:
<module 'django.conf.locale.de.formats' from '/home/tim/code/django/django/conf/locale/de/formats.pyc'>

+ [<module 'i18n.other.locale.de.formats' from '/home/tim/code/django/tests/i18n/other/locale/de/formats.pyc'>,
- [<module 'django.conf.locale.de.formats' from '/home/tim/code/django/django/conf/locale/de/formats.pyc'>]
? ^

+  <module 'django.conf.locale.de.formats' from '/home/tim/code/django/django/conf/locale/de/formats.pyc'>]
? ^

timgraham reopened this May 19, 2014

@timgraham timgraham and 1 other commented on an outdated diff May 19, 2014

django/utils/formats.py
@@ -47,10 +47,15 @@ def iter_format_modules(lang, format_module_path=None):
Does the heavy lifting of finding format modules.
"""
if check_for_language(lang):
- format_locations = ['django.conf.locale.%s']
- if format_module_path:
- format_locations.append(format_module_path + '.%s')
- format_locations.reverse()
+ format_locations = []
+ if settings.FORMAT_MODULE_PATH:
@timgraham

timgraham May 19, 2014

Owner

I'm not sure the usage of settings.FORMAT_MODULE_PATH is correct here. Now the format_module_path keyword argument is no longer used (thus the failing test, I think).

@mbrochh

mbrochh May 19, 2014

Contributor

I'll look into this ASAP
On 19 May 2014 20:25, "Tim Graham" notifications@github.com wrote:

In django/utils/formats.py:

@@ -47,10 +47,15 @@ def iter_format_modules(lang, format_module_path=None):
Does the heavy lifting of finding format modules.
"""
if check_for_language(lang):

  •    format_locations = ['django.conf.locale.%s']
    
  •    if format_module_path:
    
  •        format_locations.append(format_module_path + '.%s')
    
  •        format_locations.reverse()
    
  •    format_locations = []
    
  •    if settings.FORMAT_MODULE_PATH:
    

I'm not sure the usage of settings.FORMAT_MODULE_PATH is correct here.
Now the format_module_path keyword argument is no longer used (thus the
failing test, I think).


Reply to this email directly or view it on GitHubhttps://github.com/django/django/pull/1236/files#r12791126
.

@timgraham timgraham and 1 other commented on an outdated diff May 19, 2014

tests/utils_tests/test_formats.py
@@ -0,0 +1,63 @@
+"""Tests for ``django/utils/formats.py``."""
@timgraham

timgraham May 19, 2014

Owner

Might be better to put these tests with the existing tests for the function in tests/i18n/tests.py

@mbrochh

mbrochh May 20, 2014

Contributor

I think when I started this patch I was not aware of the existing test. Then I looked hard at the structure of all those tests and it seemed to me being obvious that a test for utils/formats.py should live in utils_tests/test_formats.py.

I'm a bit confused now about what to do next. I could either turn the tests here into methods on the existing i18n.tests.FormattingTests test case or I could move the tests test_iter_format_modules and test_iter_format_modules_stability into my new class here.

What do you suggest?

@timgraham

timgraham May 20, 2014

Owner

If it were me, I would put new tests along with the old ones.

p.s. please run flake8 and remove unused imports

@mbrochh mbrochh commented on the diff May 20, 2014

django/utils/formats.py
- if check_for_language(lang):
- format_locations = ['django.conf.locale.%s']
- if format_module_path:
- format_locations.append(format_module_path + '.%s')
- format_locations.reverse()
- locale = to_locale(lang)
- locales = [locale]
- if '_' in locale:
- locales.append(locale.split('_')[0])
- for location in format_locations:
- for loc in locales:
- try:
- yield import_module('%s.formats' % (location % loc))
- except ImportError:
- pass
+ if not check_for_language(lang):
@mbrochh

mbrochh May 20, 2014

Contributor

I reversed the if-check at the beginning so that we can un-indent the whole code by one. Much nicer to read that way.

@mbrochh mbrochh commented on the diff May 20, 2014

django/utils/formats.py
- format_locations.append(format_module_path + '.%s')
- format_locations.reverse()
- locale = to_locale(lang)
- locales = [locale]
- if '_' in locale:
- locales.append(locale.split('_')[0])
- for location in format_locations:
- for loc in locales:
- try:
- yield import_module('%s.formats' % (location % loc))
- except ImportError:
- pass
+ if not check_for_language(lang):
+ return
+
+ if format_module_path is None:
@mbrochh

mbrochh May 20, 2014

Contributor

Somehow, the parameter format_module_path was never used, only in that one failing test in i18n/tests.py - that test should have used with self.settings(FORMAT_MODULE_PATH='foobar') instead of passing in this parameter.

However, that parameter must be there for some reason and other people might use it in their codebases, so I changed the code slightly to honour the parameter again. If the parameter is given, we ignore the setting.

@timgraham this way the failing test passes. Do we need to document that format_module_path parameter and hint that it would override the FORMAT_MODULE_PATH setting when given? For people who mess around in undocumented internals? :)

@timgraham

timgraham May 20, 2014

Owner

I would check git blame to see if there's any indication of why format_module_path is there. I don't think we're under any obligation to keep it, as iter_format_modules isn't a documented function as you noted.

@mbrochh mbrochh commented on an outdated diff May 20, 2014

tests/i18n/tests.py
- de_format_mod = import_module('django.conf.locale.de.formats')
- self.assertEqual(list(iter_format_modules('de')), [de_format_mod])
- test_de_format_mod = import_module('i18n.other.locale.de.formats')
- self.assertEqual(list(iter_format_modules('de', 'i18n.other.locale')), [test_de_format_mod, de_format_mod])
+ result = list(iter_format_modules('de'))
+ self.assertEqual(result, [default_mod], msg=(
+ 'Should return the correct deault module when no setting'
+ ' is set'))
+
+ result = list(iter_format_modules('de', 'i18n.other.locale'))
+ expected = [test_mod, default_mod]
+ self.assertEqual(result, expected, msg=(
+ 'When setting is set, should return the given module and'
+ ' the default module'))
+
+ setting = ['i18n.other.locale', 'i18n.other2.locale', ]
@mbrochh

mbrochh May 20, 2014

Contributor

the two tests above have been here before. this is the new test that uses a list instead of a string

Contributor

mbrochh commented May 20, 2014

@timgraham alright, the odyssey continues. I have deleted my tests and the fake format modules that I had added and I have moved the test that checks for the new functionality into the existing test function. I also created a second fake format module next to the one that we already have at i18n/other2

I ran the whole test suite: Ran 6924 tests in 286.793s OK (skipped=537, expected failures=8)

Note: I just saw that you merged some flake8 fixes - those might be lost now becuase I ammended my commit and forced pushed into my branch. I checked i18n/tests.py and utils/formats.py for flake8 issues but could find none.

@timgraham timgraham commented on an outdated diff May 20, 2014

docs/ref/settings.txt
@@ -1378,6 +1378,20 @@ like::
__init__.py
formats.py
+.. versionchanged:: 1.8
+
+ You can also set this setting to a list of Python paths, for example::
+
+ FORMAT_MODULE_PATH = [
+ 'mysite.formats',
+ 'some_app.formats',
+ ]
+
+ When Django searches for a certain format, it will go through all given
+ Python paths until it finds a module that actually defines the given
+ format. This means that formats defined in packages farther up in the list
+ would take precendence over the same formats in packages farther down.
@timgraham

timgraham May 20, 2014

Owner

would -> will

@timgraham timgraham commented on an outdated diff May 20, 2014

docs/topics/i18n/formatting.txt
- FORMAT_MODULE_PATH = 'mysite.formats'
+.. versionchanged:: 1.8
+
+ The ability to specify FORMAT_MODULE_PATH as a list was added. Previously,
@timgraham

timgraham May 20, 2014

Owner

:setting:

@timgraham timgraham commented on an outdated diff May 20, 2014

tests/i18n/tests.py
with translation.override('de-at', deactivate=True):
- de_format_mod = import_module('django.conf.locale.de.formats')
- self.assertEqual(list(iter_format_modules('de')), [de_format_mod])
- test_de_format_mod = import_module('i18n.other.locale.de.formats')
- self.assertEqual(list(iter_format_modules('de', 'i18n.other.locale')), [test_de_format_mod, de_format_mod])
+ result = list(iter_format_modules('de'))
+ self.assertEqual(result, [default_mod], msg=(
+ 'Should return the correct deault module when no setting'
@timgraham

timgraham May 20, 2014

Owner

default* (and I think these would be better as inline comments -- the msg kwarg kinda clutters things IMO and probably doesn't offer much benefit)
also, I would chop the intermediate variables and just multiline the assertEqual if you think the line is too long (although the 80 character limit is a soft limit in code)

assertEqual(
    list(iter_format_modules('de')),
    [default_mod])
)

"One big exception to PEP 8 is our preference of longer line lengths. We’re well into the 21st Century, and we have high-resolution computer screens that can fit way more than 79 characters on a screen. Don’t limit lines of code to 79 characters if it means the code looks significantly uglier or is harder to read." https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/coding-style/

@mbrochh mbrochh Fixed #20477: Allowed list of modules for FORMAT_MODULE_PATH
Previously the FORMAT_MODULE_PATH setting only accepted one string (dotted
module path). A feature has been added to also allow a list of strings.

This is useful when using several reusable third party apps that define new
formats. We can now use them all and we can even override some of the formats
by providing a project-wide format module.
06cf6e9
Contributor

mbrochh commented May 20, 2014

@timgraham added your remarks.

Owner

timgraham commented May 20, 2014

buildbot, test this please.

Owner

timgraham commented May 20, 2014

Looks good. Test failure on Jenkins are unrelated. I'll merge it soon.

Contributor

mbrochh commented May 20, 2014

@timgraham thank you so so so much for your patience and guidance. I guess this pull request kind of defeated the whole purpose of open source - if I had told you my feature idea and you had implemented it, the whole thing would have been done in 30 minutes one year ago.

But then again, I think I learned a lot along the way and will definitely be much much much more precise when submitting pull requests in the future.

Owner

timgraham commented May 21, 2014

You're welcome, look forward to your future contributions. :-)

merged in bb0a9a0.

timgraham closed this May 21, 2014

mbrochh deleted the unknown repository branch May 26, 2014

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