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

Add box_namespace to track nested Box names #227

Closed
wants to merge 2 commits into from

Conversation

JacobHayes
Copy link
Contributor

I'd like to track the "namespace" of nested boxes, which is hard to do in a subclass because of the (necessary) recursive conversions during __init__ and other actions. To handle this, I added a new box_namespace parameter, which is tracked in _box_config, to allow propagating and extending the namespace as we create nested boxes.

Closes #226

@JacobHayes
Copy link
Contributor Author

JacobHayes commented Aug 10, 2022

FYI: I think newer versions of mypy are a bit more strict about dict.__ior__, so I'll include a quick "fix" for that in this PR (that's why the first commit "failed").

@cdgriffith cdgriffith changed the base branch from master to develop August 16, 2022 02:06
@cdgriffith
Copy link
Owner

Hey @JacobHayes thanks for the great add!

This is something I have struggled with before #15 so I'm going to do some prodding to see if I can make it unravel in any of the weird ways that bit me before (if I can recall them...), but I'd be very happy if this works solidly!

@JacobHayes
Copy link
Contributor Author

Sounds great! Feel free to add any new tests and I can take a stab at them if they don't work right away.

@@ -436,12 +444,13 @@ def __setstate__(self, state):

def __get_default(self, item, attr=False):
default_value = self._box_config["default_box_attr"]
config = self.__box_config(extra_namespace=item)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's super handy to do this at the start of the function here. But for now will need to keep it in each if block, as this would slow down processes that don't use one of the defaults that require it.

This code wasn't designed for speed from the start, but enough people use it in "semi performant" applications I try to think a little bit about not slowing down some paths they might have optimized for at least 🤷

@@ -482,20 +493,21 @@ def __convert_and_store(self, item, value):
if self._box_config["box_intact_types"] and isinstance(value, self._box_config["box_intact_types"]):
return super().__setitem__(item, value)
# This is the magic sauce that makes sub dictionaries into new box objects
config = self.__box_config(extra_namespace=item)
Copy link
Owner

@cdgriffith cdgriffith Aug 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with this needing to be left under the if / elif blocks as above, and will stop repeating myself here, you get the picture ;)

@cdgriffith
Copy link
Owner

My two thoughts tonight before sleep takes me:

  1. (Not required for round 1), look into expanding this to work with BoxList as well.
  2. It's great to track this, but I feel if we add it, do we want to "do" something with it. By adding a box_history() or something so it's more accessible. Though at the same time, I feel that if you need this, you're building on top of Box itself somehow so are probably okay working with the config.... hmmm

@JacobHayes
Copy link
Contributor Author

JacobHayes commented Aug 16, 2022

(Not required for round 1), look into expanding this to work with BoxList as well.

How do you think Boxes inside BoxLists should be named? Eg: should {"a": {"b": [{"c": 5}]}} be something like ("a", "b", 0, "c")?

It's great to track this, but I feel if we add it, do we want to "do" something with it.

Yeah, this is a good point. I'm internally subclassing Box and using this via the config as you say (and then exposing those Boxes to users as an "implementation" detail). I'm ok with using the config, but could see use in a more "public" interface. Were you thinking a box_history function (eg: from box import box_history) or a method on the Box class? I know we need to be careful about name collisions.

(and time for me to go to bed too haha)

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.

Support for tracking the box "namespace"
2 participants