Opened 3 years ago
Closed 3 years ago
#32941 closed Cleanup/optimization (fixed)
Consider removing unused reverse parameter of utils.formats.get_format_modules
Reported by: | Keryn Knight | Owned by: | Keryn Knight |
---|---|---|---|
Component: | Utilities | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Currently the implementation is:
def get_format_modules(lang=None, reverse=False): """Return a list of the format modules found.""" if lang is None: lang = get_language() if lang not in _format_modules_cache: _format_modules_cache[lang] = list(iter_format_modules(lang, settings.FORMAT_MODULE_PATH)) modules = _format_modules_cache[lang] if reverse: return list(reversed(modules)) return modules
but I'd suggest that removing reverse
as a parameter should ultimately have no affect, as it's only used by tests.i18n.tests.FormattingTests.test_get_format_modules_stability
thus the method could become:
def get_format_modules(lang=None): """Return a list of the format modules found.""" if lang is None: lang = get_language() if lang not in _format_modules_cache: _format_modules_cache[lang] = list(iter_format_modules(lang, settings.FORMAT_MODULE_PATH)) modules = _format_modules_cache[lang] return modules
leaving any downstream caller to do the reversed(modules)
call themselves, should they need to. Arguably they might not need/want a list anyway...
Patch will come to demonstrate that with the adjustment of the single test everything should be OK to consider removing it. The test would pass whether or not I reverse the output, but I'm opting to keep intent of the test method the same...
Change History (6)
comment:1 by , 3 years ago
comment:2 by , 3 years ago
Triage Stage: | Unreviewed → Accepted |
---|
Accepting as get_format_modules()
is undocumented so I don't have any concerns about backward compatibility.
comment:3 by , 3 years ago
FYI, #15129 added a regression test to fix a bug when passing reverse=True
.
comment:4 by , 3 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:5 by , 3 years ago
I tracked it down - the reverse
argument has been unused since 0d8b523422fda71baa10807d5aebefd34bad7962.
PR is here: https://github.com/django/django/pull/14659
Awaiting CI results to see whether or not it could move to accepted.