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

Config attribute setting needs to work better #413

Closed
bitprophet opened this Issue Dec 12, 2016 · 1 comment

Comments

Projects
None yet
1 participant
@bitprophet
Member

bitprophet commented Dec 12, 2016

Background

As of 0.14, the "use either attribute or dict syntax" behavior for Context/Config/DataProxy functions reasonably well, except for when setting brand new config keys that don't already exist, in which case the attribute approach doesn't work - it simply sets a real attribute on whatever DataProxy is being touched.

This is very foot-gunny as it leads to difficult to understand/troubleshoot behavior - no error, and attribute access sometimes works after due to happenstance, but any e.g. cloning of the config, or dict access, or etc, does not preserve the attribute; tests like "if key in config" fail; etc.

What should happen is that new attribute accesses do set config values...but only from "outside" the class, and not in, say, __init__, otherwise there's a chicken/egg problem re: how to set initial, real attributes.

Strongly related is an issue where the "real attributes win over config values w/ same name, during attribute access" behavior - changed for 0.14 - is inconsistent because it only looks at self/type, and not supertypes, so it still doesn't quite work when one is dealing with Context or Config (versus a nested config values, which becomes a 'raw' DataProxy).

Solutions

  • 'Lock'/'unlock' attributes-become-config behavior, e.g. in a decorator around __init__ or other methods
    • Should work pretty solidly
    • But requires implementers of subclasses to remember to use the decorator anyplace they create new attributes
    • And is moderate bookkeeping, i.e. keep an attribute around as state for whether the obj is in "let me touch attrs or not" mode
      • which then rolls into the supertypes issue above, because if that's not working, Context and Config both need their own initializations of the bookkeeping attribute
      • besides that, it's just more state that can get set incorrectly, or cause threading issues, etc
    • Main upside: when decorator is in place, decorated code can simply forget about the whole deal and use attributes normally
  • Just remember to use object.__setattr__ anytime one makes new attributes, which skips our "clever" local implementation entirely.
    • Bit more footgunny for devs insofar as it's now a lot easier to forget (any attribute creation vs just set & forget once per method, with the above decorator approach)
    • Also just more verbose
    • Might have some quirks on eg Python 3 (tho I can't find the NOTE: I thought I left about this in the deep past...)
    • But it's less bookkeeping

Neither of those is a clear winner but I'm leaning towards the latter (less code and state is better) so I may try it first to see just how verbose it is and whether it even works as expected.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Dec 12, 2016

object.__setattr__ appears to do the job, including under Python 3. Yea, it makes Config.__init__ a good amount more verbose (we initialize a few dozen attributes there) but I still feel better about doing it this way vs introducing more custom state crap.

My real world code that was working around this via dict-style setting, also appears to work fine with this change in place. Yay.

bitprophet added a commit that referenced this issue Dec 12, 2016

Failing test proving the other half of #413
Namely that 'is it an attr?' test isn't rigorous enough
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment