[Django] #32941: Consider removing unused reverse parameter of utils.formats.get_format_modules
Django
utils.formats.get_format_modules
------------------------------------------------+------------------------
Reporter: Keryn Knight | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: dev
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
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...
--
Ticket URL: <https://code.djangoproject.com/ticket/32941>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
utils.formats.get_format_modules
Reporter: Keryn Knight | Owner: nobody
Cleanup/optimization |
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Keryn Knight):
PR is here: https://github.com/django/django/pull/14659
Awaiting CI results to see whether or not it ''could'' move to accepted.
--
Ticket URL: <https://code.djangoproject.com/ticket/32941#comment:1>
Django
utils.formats.get_format_modules
Reporter: Keryn Knight | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: dev
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Nick Pope):
* stage: Unreviewed => Accepted
Comment:
Accepting as `get_format_modules()` is undocumented so I don't have any
concerns about backward compatibility.
--
Ticket URL: <https://code.djangoproject.com/ticket/32941#comment:2>
Django
utils.formats.get_format_modules
Reporter: Keryn Knight | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Jerdonek):
FYI, #15129 added a regression test to fix a bug when passing
`reverse=True`.
--
Ticket URL: <https://code.djangoproject.com/ticket/32941#comment:3>
Django
utils.formats.get_format_modules
Reporter: Keryn Knight | Owner: Keryn
Type: | Knight
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* owner: nobody => Keryn Knight
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32941#comment:4>
Django
utils.formats.get_format_modules
Reporter: Keryn Knight | Owner: Keryn
Type: | Knight
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nick Pope):
I tracked it down - the `reverse` argument has been unused since
0d8b523422fda71baa10807d5aebefd34bad7962.
--
Ticket URL: <https://code.djangoproject.com/ticket/32941#comment:5>
Django
utils.formats.get_format_modules
Reporter: Keryn Knight | Owner: Keryn
Type: | Knight
Component: Utilities | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"59942a66ceb79868cb91844df3a72a24c63e39fa" 59942a66]:
{{{
#!CommitTicketReference repository=""
revision="59942a66ceb79868cb91844df3a72a24c63e39fa"
Fixed #32941 -- Removed get_format_modules()'s unused reverse argument.
Unused since 0d8b523422fda71baa10807d5aebefd34bad7962.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32941#comment:6>