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

setdefault, or some superior functionality as part of transform() #95

Open
itamarst opened this issue Feb 3, 2017 · 5 comments
Open

Comments

@itamarst
Copy link
Contributor

itamarst commented Feb 3, 2017

Consider the following code:

class ApplicationState(PClass):
    shared_resources = pset_field(Injectable)
    # ...

class ExtractedState(PClass):
    applications = pmap_field(str, ApplicationState)

def extract(tfstate):
    result = {}

    for mod in tfstate['modules']:
        for injectable in _extract_resources_from_module(mod):
            app_state = result.setdefault(injectable.app,
                                          {"shared_resources": set(),
                                           "service_resources": {}})
            app_state["shared_resources"].add(injectable)

   return ExtractedState.create(freeze({"applications": result}))

Constructing the pyrsistent objects is quite annoying, which is why I'm doing it with mutable objects, due to lack of setdefault or "create a default object in transform()". Ideally I'd do:

extracted_state = ExtractedState()
extracted_state = extracted_state.transform(
    ["applications", "newapp", "shared_resources"], lambda ps: ps.add(obj))

but since newapp doesn't exist it blows up...

@tobgu
Copy link
Owner

tobgu commented Feb 7, 2017

Seems cool, I can definitely see the use case.

At first I was thinking that the type information should be possible to use for auto building default structures. It seems to me that it would impose a lot of restrictions since the type information would have to be unambiguous (a type is specified, only one type is specified, the type is not abstract, etc.).

Furthermore I guess it may not always be the case that you want to do the exact same thing for every transformation involving a particular type. This means that the behaviour should probably not be tied to the type/class at all but instead be supplied as input to the transform operation. I guess this was what you were thinking as well since you mentioned setdefault.

What about making it possible to pass a function default(path) to transform that gets called with the current path (a list) as argument when a node specified in the transformation is missing? It would then return a node appropriate for the path (or ignore the path completely and just return a node if considered OK/safe).

extracted_state = extracted_state.transform(
    ["applications", "newapp", "shared_resources"], lambda ps: ps.add(obj), default=node_constructor_fn)

@itamarst
Copy link
Contributor Author

itamarst commented Feb 7, 2017

That seems like it would require enough typing that it wouldn't save any time.

In practice with setdefault the vast majority of the time you just want an empty data structure ([], or default object with no arguments, etc.) So maybe just:

extracted_state = extracted_state.transform(
    ["applications", "newapp", "shared_resources"], lambda ps: ps.add(obj), add_defaults=True)

@itamarst
Copy link
Contributor Author

itamarst commented Feb 7, 2017

(Or some other, better, argument name.)

@tobgu
Copy link
Owner

tobgu commented Feb 7, 2017

The only problem with that as I see it is that there are a lot of cases where such simplification would not be usable at all. For example when applying a transform to a data structure without type information (nested pvectors, pmaps and psets for example).

It's of course possible to combine the two suggestions and have a parameter (would calling it setdefault hint about the intent or just make it confusing?) that could either be True/False or a function as the one I suggested above. If set to True it would assume/require proper type information to be present and fail otherwise.

@itamarst
Copy link
Contributor Author

itamarst commented Feb 7, 2017

Maybe instead of an option to transform() it could be a top-level function, e.g. 'add()`? This would:

  1. Allow more flexibility in how it's defined without worrying about backwards compat.
  2. There are other use cases that could be covered, then. E.g. notice how adding an item to a nested set is also verbose in examples above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants