Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure consistent output ordering #96

Merged
merged 2 commits into from
Aug 4, 2020

Conversation

PeterJCLaw
Copy link
Contributor

Since the output file is one which we assume that people will put into version control, they're probably also going to want it to be stable and have CI validating it being up to date.

Since the output file is one which we assume that people will put
into version control, they're probably also going to want it to be
stable and have CI validating it being up to date.
@graingert
Copy link
Contributor

graingert commented Jul 23, 2020

I think a neater fix is to use an OrderedDict in https://github.com/ierror/django-js-reverse/blob/master/django_js_reverse/core.py#L110-L121

eg:

def generate_json(default_urlresolver, script_prefix=None):
    if script_prefix is None:
        script_prefix = urlresolvers.get_script_prefix()
    urls = sorted(list(prepare_url_list(default_urlresolver)))

    return collections.OrderedDict(
        [
            (
                'urls',
                [
                    [
                        force_text(name),
                        [
                            [force_text(path), [force_text(arg) for arg in args]]
                            for path, args in patterns
                        ],
                    ]
                    for name, patterns in urls
                ],
            ),
            ('prefix', script_prefix),
        ]
    )

because there's only 2 keys in the whole data structure

@coveralls
Copy link

coveralls commented Jul 23, 2020

Coverage Status

Coverage increased (+0.002%) to 99.087% when pulling 93dbc29 on PeterJCLaw:consistent-output into ba8ce52 on ierror:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.085% when pulling f089577 on PeterJCLaw:consistent-output into ba8ce52 on ierror:master.

@PeterJCLaw
Copy link
Contributor Author

PeterJCLaw commented Jul 23, 2020

I think a neater fix is to use an OrderedDict in https://github.com/ierror/django-js-reverse/blob/master/django_js_reverse/core.py#L110-L121

because there's only 2 keys in the whole data structure

Yeah, I considered using an OrderedDict but decided against because sort_keys will ensure that the whole structure is always consistent, rather than making this a feature which the structure itself needs to maintain (I realise the structure already kinda has to do this for the list parts, so maybe this isn't a strong enough reason).
I agree it doesn't feel obvious that the _safe_json helper handles this though.

@PeterJCLaw
Copy link
Contributor Author

Anyway, 93dbc29 changes to using an OrderedDict.

@PeterJCLaw PeterJCLaw changed the title Use sort_keys to ensure consistent output Ensure consistent output ordering Jul 27, 2020
@PeterJCLaw
Copy link
Contributor Author

@graingert just so I know roughly what to expect -- what's the normal release cadence for this project?

@graingert graingert merged commit fba9379 into ierror:master Aug 4, 2020
@PeterJCLaw PeterJCLaw deleted the consistent-output branch September 1, 2020 17:59
@PeterJCLaw PeterJCLaw restored the consistent-output branch September 1, 2020 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants