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

BinData can't be instantiated #9

Closed
jmuhlich opened this issue Jul 16, 2020 · 4 comments
Closed

BinData can't be instantiated #9

jmuhlich opened this issue Jul 16, 2020 · 4 comments

Comments

@jmuhlich
Copy link
Collaborator

I don't think dataclasses can usefully subclass a non-dataclass or really any non-cooperative class. For example our BinData ultimately subclasses str and I discovered there's no way to actually instantiate one due to errors from either the str constructor or the pydantic validator. How would the core string value actually get passed in here, anyway? The other subclasses of ConstrainedStr seem OK since they aren't also dataclasses. Maybe BinData should be a top-level class with an attribute value: base64Binary rather than inheriting.

@jmuhlich
Copy link
Collaborator Author

Simply making that proposed change (moving the content to a value attribute) makes 14/32 of the example files at https://github.com/ome/ome-model/tree/master/specification/samples/2016-06 parse without error. I didn't inspect them all to see if the returned object looks correctly structured, but it's a good start. I'll look into how to make that change in OVERRIDE.

@tlambert03
Copy link
Owner

yeah I recall some of the constrained strings (with additional properties) being hard to model... but I didn't appreciate that it couldn't even be instantiated! That'll never do! :)
how many of those files are parsing without error before that fix? (we should update the tests here to use those)

@jmuhlich
Copy link
Collaborator Author

jmuhlich commented Jul 17, 2020 via email

@tlambert03
Copy link
Owner

fwiw, I'm more than happy to use whatever "ugliness" is necessary for now to just get the output model working. So if using the current OVERRIDE pattern is not working, and you want to just put a conditional somewhere in the body of the code with a "# FIXME:" or something like that. That's fine with me and we can refactor later

jmuhlich added a commit to jmuhlich/ome-types that referenced this issue Jul 21, 2020
In the schema BinData is an extension of string, but modeling this in python
as a subclass of str with extra constructor args is not feasible. Instead
this change moves the string content to a `value` attribute which aligns
with how our to_dict converter presents element text content.

Fixes tlambert03#9
jmuhlich added a commit to jmuhlich/ome-types that referenced this issue Jul 21, 2020
In the schema BinData is an extension of string, but modeling this in python
as a subclass of str with extra constructor args is not feasible. Instead
this change moves the string content to a `value` attribute which aligns
with how our to_dict converter presents element text content.

Fixes tlambert03#9
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

No branches or pull requests

2 participants