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 Keryn Knight, 3 years ago

PR is here: https://github.com/django/django/pull/14659
Awaiting CI results to see whether or not it could move to accepted.

comment:2 by Nick Pope, 3 years ago

Triage Stage: UnreviewedAccepted

Accepting as get_format_modules() is undocumented so I don't have any concerns about backward compatibility.

comment:3 by Chris Jerdonek, 3 years ago

FYI, #15129 added a regression test to fix a bug when passing reverse=True.

comment:4 by Jacob Walls, 3 years ago

Owner: changed from nobody to Keryn Knight
Status: newassigned

comment:5 by Nick Pope, 3 years ago

I tracked it down - the reverse argument has been unused since 0d8b523422fda71baa10807d5aebefd34bad7962.

comment:6 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 59942a66:

Fixed #32941 -- Removed get_format_modules()'s unused reverse argument.

Unused since 0d8b523422fda71baa10807d5aebefd34bad7962.

Note: See TracTickets for help on using tickets.
Back to Top