Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#24328 closed Cleanup/optimization (fixed)

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

Reported by: Anssi Kääriäinen Owned by: nobody
Component: Database layer (models, ORM) Version: 1.8alpha1
Severity: Release blocker Keywords: 1.8-beta
Cc: pirosb3 Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (8)

comment:1 by Tim Graham, 10 years ago

Cc: pirosb3 added
Keywords: 1.8-beta added
Triage Stage: UnreviewedAccepted

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?

comment:2 by pirosb3, 10 years ago

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

comment:3 by Tim Graham, 10 years ago

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?

comment:4 by pirosb3, 10 years ago

Hi Tim

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

comment:5 by Tim Graham, 10 years ago

Has patch: set
Severity: NormalRelease blocker

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

comment:6 by Tim Graham, 10 years ago

Triage Stage: AcceptedReady for checkin

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

comment:7 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: newclosed

In bad5f262bf4a17f157808ec1aa225ba9c94c1eee:

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

comment:8 by Tim Graham <timograham@…>, 10 years ago

In 6f03a4ca91ed22b2ea42d5159a1ed00f347cf9aa:

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

Backport of bad5f262bf4a17f157808ec1aa225ba9c94c1eee from master

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