8000 Default values for frozen attrs classes · Issue #263 · textX/textX · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Default values for frozen attrs classes #263

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.

Sign up for GitHub

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

Jump to bottom
Open
markusschmaus opened this issue Jun 11, 2020 · 6 comments
Open

Default values for frozen attrs classes #263

markusschmaus opened this issue Jun 11, 2020 · 6 comments
Labels
feature

Comments

@markusschmaus
Copy link
Contributor
markusschmaus commented Jun 11, 2020 •
edited
Loading

Base type attribute are initialized as None with the comment that default values can be specified via object processors.

textX/textx/metamodel.py

Lines 460 to 464 in c8a49a2

# Set base type attribute to None initially
# in order to be able to detect if an optional
# values are given in the model. Default values
# can be specified using object processors.
setattr(obj, attr.name, None)

However for frozen attrs classes, object processors are run too late to actually change the value of the attribute, so there is no obvious way to provide a default value for such classes.

One way to solve this would be to filter out any None values when constructing the dictionary passed to __init__ and use the standard mechanisms for default values.

textX/textx/model.py

Lines 836 to 840 in c8a49a2

# We shall only pass to __init__ attributes that are
# defined by the meta-model, and `parent` if applicable
attrs = {k: v for k, v in attrs.items()
if k in obj.__class__._tx_attrs
or k == 'parent'}

would become

                attrs = {k: v for k, v in attrs.items()
                         if (k in obj.__class__._tx_attrs
                         or k == 'parent')
                         and v is not None}
@igordejanovic
Copy link
Member

User have full control over user classes so the obvious and preferred way to provide defaults is in the class constructor. For dynamically created textX classes we don't have other options but to use object processors.

@markusschmaus
Copy link
Contributor Author

Sounds reasonable. The issue is that currently the defaults provided by the class constructor are not used because TextX sets missing properties explicitly to None. Thats why I suggest filtering out the None values for user classes, though this would be a change to a public API.

@igordejanovic
Copy link
Member

Yeah, it would be a change to the way it worked before. I guess that documenting the preferred way of providing the defaults might be a better approach. Something like:

class MyUserClass:
    def __init__(self, first_param, ...):
        self.fist_param = <default value> if first_param is None else first_param
....

This is a good practice anyway as the usage of default values directly in the parameter list can lead to bugs if mutable values are used.

@markusschmaus
Copy link
Contributor Author

I think best practice is to use a custom sentinel value as default rather than None, so that setting a value to None does not get confused with using the default value.

I discovered this on my own when I was bitten by some subtle bugs during a project at work, but I have since then discovered that attrs uses the same pattern with attr.NOTHING as sentinel value. Though I would always recommend to create your own sentinel value rather than reusing the sentinel value of another project. I just found this blog post which also recommends this design pattern.

It would be great if there was a way to ask textx to not set missing values to None, in order to create compatibility with attrs and enable the user to use their custom sentinel value for defaults.

@igordejanovic
Copy link
Member

Sounds like a good idea. I think it would be relatively straightforward to implement it as an optional parameter to model_from* where the default value may be None so it stays backward compatible.

@igordejanovic
Copy link
Member

OTOH, textX will only set value to None if it is missing in the input so the only benefit of providing sentinel value would be if user-classes are also instantiated by the user code outside of textX. But, if you think it would be usable I think it would be ok for textX to provide it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@igordejanovic @markusschmaus
0