The new Options._get_fields() method needs to be cleaned up

[Anssi Kääriäinen]

At least the method _get_fields() has a lot of places where the code isn't as clean as it should be.

Examples from master:

• The method has a flag export_ordered_set, but that flag is never used, and the method definitely doesn't export an ordered set.
• The comment in the first try-catch states that the method must always return a shallow copy. However, the comment doesn't make any sense in that location (the code doesn't make a shallow copy in that location)
• The method constructs an OrderedDict of field -> boolean. The boolean flag is always True and never used. It would be possible to use an ordinary list instead.
• The comment in the beginning of "if not export_ordered_set" isn't accurate, it states that

   # By default, fields contains field instances as keys and all
              # possible names if the field instance as values. When
              # _get_fields() is called, we only want to return field instances,
              # so we just preserve the keys.
  

  this comment doesn't make much sense.

Other parts of the code seem to be lot better. In addition, L592 contains f.rel.field.rel.is_hidden(). But f.rel.field.rel is always the same as f.rel. L476 states that the code calls _get_fields() with export_ordered_set=True, but that isn't actually happening.

The functionality itself seems to be fine, but there is just a lot of left-overs from internal refactorings.

[Tim Graham]

Didn't look through all these, but export_ordered_set seems to be used when calling the function recursively. Dan, do you have time to address these issues?

[pirosb3]

Hi All,

First of all, thanks for raising these issues. I'll answer in the order you listed your concerns:

• export_ordered_set is necessary: as the calls to _get_fields() are recursive, we want to keep track of the order each field is inserted. This is usually important for form builders (admin, ModelForms, ..). If throughout this process there is a model that overrides a field, then we want to make sure the order is still maintained. I perfectly agree that a list could be used in this case but I preferred using an OrderedDict because it felt better in terms of simplicity of code (at least to me, your ticket says otherwise) and complexity (constant time insertion and update). Finally, these OrderedDict instances are never returned by the public API, they are only returned by the recursive calls (the ones without export_ordered_set=True )

• I agree the comment is a bit misleading. The previous get_fields() was caching lists instead of tuples and was prone to cache manipulation and I added this comment just for reference.

• Same answer as N.1

• Very correct! This comments refers to the previous implementation. I will open a PR.

I hope my explanations were clear, but in case you would like to discuss some other changes to put in the PR please let me know.

Dan

[Tim Graham]

How about using an OrderedSet instead of OrderedDict ​like so?

Maybe we could rename export_ordered_set -- it doesn't seem to do what the name suggests. Something like recursive_call maybe?

[pirosb3]

Hi Tim

It looks great! I didn't know of the existence of OrderedSet .

[Tim Graham]

As ​Anssi's PR involves API changes, we want to make a decision on whether or not to do this for 1.8.

[Tim Graham]

It seems like this patch is ready, pending some minor cleanup of comments which I can do later today if needed.

[Tim Graham <timograham@…>]

In bad5f262bf4a17f157808ec1aa225ba9c94c1eee:

Fixed #24328 -- cleaned up Options._get_fields() implementation

[Tim Graham <timograham@…>]

In 6f03a4ca91ed22b2ea42d5159a1ed00f347cf9aa:

[1.8.x] Fixed #24328 -- cleaned up Options._get_fields() implementation

Backport of bad5f262bf4a17f157808ec1aa225ba9c94c1eee from master