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

allow from_string to be specified via metadata #624

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

Conversation

minrk
Copy link
Member

@minrk minrk commented Sep 18, 2020

avoids need to define custom Trait subclasses just to define how strings are parsed, e.g.

class C(Configurable):
    path = List().tag(config=True, from_string=os.pathsep.split)

avoids need to define custom Trait subclasses in order to register from_string

# allow from_string to be overridden via metadata
if self.metadata.get("from_string"):
self.from_string = self.metadata["from_string"]
Copy link
Contributor

@rmorshea rmorshea Sep 18, 2020

Choose a reason for hiding this comment

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

Why not look this up in the metadata dict rather than assign to an attr and find it there? Or if from_string can be a method on the class, use the metadata as a fallback. Seems like it would confusing or hard to debug if from_string in metadata were allowed to somewhat dynamically overwrite a method.

Copy link
Contributor

@rmorshea rmorshea Sep 19, 2020

Choose a reason for hiding this comment

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

Nevermind, given the point is to overwrite methods this makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's the TraitType subclass priority I was thinking of. The alternative was to implement _from_string private method that does the lookup, but I thought that might get confusing.

@minrk
Copy link
Member Author

minrk commented Oct 15, 2020

I was about to merge this when I realized the argument should probably be a Bunch with access to the trait instance itself like we do with all other hooks, to grant access to other tags, default values, etc.

@minrk
Copy link
Member Author

minrk commented Oct 15, 2020

Hm, but that makes it not have the same signature as the method. Maybe I should make the signature (trait, string) so it's the same as the method? I'm not sure.

@rmorshea
Copy link
Contributor

rmorshea commented Oct 15, 2020

I think making it (trait, string) would be best. Simpler and more inline with the signature of the method.

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.

2 participants