Skip to content
This repository was archived by the owner on Apr 22, 2020. It is now read-only.

Define typed struct fields with context managers #76

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

idanarye
Copy link

@idanarye idanarye commented May 8, 2018

The magic here is so strong you can start calling me "Idandalf the White", but this will allow much more elegant definition of typed structs.

For example, we have something like this in our mayhem configuration:

    def numeric_field(field_type, **kwargs):
        field = ts.Field(field_type, **kwargs)
        field.add_validation(lambda n: 0 <= n, lambda: NegativeNumbersInMayhemConfig(field_name=field.name))
        for convert_from in [int, float, str]:
            if convert_from is not field.type:
                field.add_conversion(convert_from, field.type)
        return field


    class MayhemConfig(ts.TypedStruct):
        errors = numeric_field({str: float})
        sleeping_intervals = numeric_field([Duration])
        extra_action_sleeping_intervals = numeric_field([Duration])
        protection_from_removal_period = numeric_field([Duration])
        rebuild_while_down_probability = numeric_field(float, default=0.0)
        extra_action_period = numeric_field(Duration, default=0)
        shrink_default_filesystem = numeric_field(float, default=0.0)
        wait_for_all_actions_end_probablility = numeric_field(float, default=0.0)

With this new feature, we could make it look like this:

    def numeric_field(field):
        field.add_validation(lambda n: 0 <= n, lambda: NegativeNumbersInMayhemConfig(field_name=field.name))
        for convert_from in [int, float, str]:
            if convert_from is not field.type:
                field.add_conversion(convert_from, field.type)


    class MayhemConfig(ts.TypedStruct):
        with FIELD_SETTING(numeric_field):
            errors = {str: float}
            sleeping_intervals = [Duration]
            extra_action_sleeping_intervals = [Duration]
            protection_from_removal_period = [Duration]
            with DEFAULT(0):
                rebuild_while_down_probability = float
                extra_action_period = Duration
                shrink_default_filesystem = float
                wait_for_all_actions_end_probablility = float

@idanarye
Copy link
Author

@koreno, @doroncohen - I need your opinion.

I'm going to need to add some configuration fields on the fields, to allow customizing some of TypedStruct functionalities. Specifically I have two in mind right now:

  • I want to add __hash__ to TypedStruct, but there may be fields that are unhashable by default. I need to be able to tell TypedStruct to ignore them - or even to specify a custom hash function for them:
     class Foo(TypedStruct):
     	name = str
     	data = [str]
     	data.hash = None  # disable hashing for this field
    
     # Or
    
     class Foo(TypedStruct):
     	name = str
     	data = [str]
     	data.hash = lambda data: hash(tuple(data))  # convert to tuple and then hash
  • Some fields are too big to be printed in `repr~, so I want to be able to either omit them or create custom print function for them:
     class Foo(TypedStruct):
     	name = str
     	data = [str]
     	data.repr = None  # disable printing this field in __repr__
    
     # Or
    
     class Foo(TypedStruct):
     	name = str
     	data = [str]
     	data.repr = lambda data: '%s items' % len(data)  # just print the number of items

Now, regarding the new syntax - I'm pondering how to approach it:

  1. Give them their own context managers:
    class Foo(TypedStruct):
    	name = str
    	with HASH(None), REPR(NONE):
    		data = [str]
    This will be consistent, but I'm not sure if I want to start giving a context manager for each and every configuration (there may be more in the future)
  2. Have a single context manager CONFIG with keyword arguments:
    class Foo(TypedStruct):
    	name = str
    	with CONFIG(hash=None, repr=None):
    		data = [str]
    If I pick this approach - should I merge DEFAULT into it as well? I think DEFAULT deserve it's own context manager...
  3. Don't give them context managers, let users do it manually with APPLY:
    def set_hash_and_repr(field):
    	field.hash = None
    	field.repr = None
    
    class Foo(TypedStruct):
    	name = str
    	with APPLY(set_hash_and_repr):
    		data = [str]
    This is also an option, that might be enough for the beginning...

@doron-cohen
Copy link

.hash and .repr:
I like the approach but wondering if it should be assigned with NotImplemented instead of None.

About your second question I would go with the 2nd approach it is much clearer for this scenario. if you needed to set an alternative repr and hash for fields (other than None) I would go with the third option because the lambdas could make it messy.

@idanarye
Copy link
Author

How do you feel about this syntax?

class Foo(TypedStruct):
    a = int; DEFAULT(5)
    b = float; CONVERTIBLE_FROM(int, str)

    # Or, if there are many fields to set:
    c = str
    DEFAULT(20)
    CONVERTIBLE_FROM(int, str)
    VALIDATION(lambda s: 2 <= len(s))

@idanarye
Copy link
Author

I like the approach but wondering if it should be assigned with NotImplemented instead of None.

NotImplemented feels like more "if someone tries to use this throw an exception" than a "skip this one".

@doron-cohen
Copy link

doron-cohen commented May 24, 2018

I can bare with the semicolon syntax although I don't think linters like the use of it. second option is not optimal, maybe use indentation in some way like:

class Foo(TypedStruct):
  c = str
    DEFAULT('20')
    ...

you may have to use a tuple for that

@idanarye
Copy link
Author

If I used a tuple, completion would no longer work. That was also the problem with using ts.Field.

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

Successfully merging this pull request may close these issues.

2 participants