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

Make annotations work #21

Merged
merged 1 commit into from
Jul 24, 2020
Merged

Conversation

jmuhlich
Copy link
Collaborator

This was straightfoward except for the XMLAnnotation. I wanted to present the "any" XML sub-tree as something usable and not just a stringified serialization that would have to be re-parsed anyway. An xml.etree.ElementTree.Element object is perfect, however I had trouble getting the validation to work with that as the attribute type. I did have to tell Pydantic to allow arbitrary classes as types, but for some reason the isinstance check it does fails mysteriously. I typed it Any just to move on but it's something to revisit later.

I had to disable the mypy warn_unused_ignores setting because it's triggered by value: Any = _no_default # type: ignore. With some work on the code generation that could be added back in later.

@codecov-commenter
Copy link

codecov-commenter commented Jul 23, 2020

Codecov Report

Merging #21 into master will increase coverage by 0.26%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #21      +/-   ##
==========================================
+ Coverage   95.20%   95.46%   +0.26%     
==========================================
  Files           1        1              
  Lines         396      419      +23     
==========================================
+ Hits          377      400      +23     
  Misses         19       19              
Impacted Files Coverage Δ
src/ome_autogen.py 95.46% <100.00%> (+0.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4012024...65ba27b. Read the comment docs.

Copy link
Owner

@tlambert03 tlambert03 left a comment

Choose a reason for hiding this comment

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

wow. amazing! very clever with the etree strategy. so you fixed pretty much everything but one that will probably never work, yeah?

I did have to tell Pydantic to allow arbitrary classes as types

where is this done?

@jmuhlich
Copy link
Collaborator Author

I did have to tell Pydantic to allow arbitrary classes as types

where is this done?

It was part of my experiment in typing XMLAnnotation.value as ElementTree.Element, but since that failed for other reasons you won't see it in the committed code. You use the Config nested class as documented here:
https://pydantic-docs.helpmanual.io/usage/types/#arbitrary-types-allowed
For dataclasses you need to use the config kwarg to the @dataclass decorator instead, so it would look like this:

class Config:
    arbitrary_types_allowed = True

@dataclass(config=Config)
class XMLAnnotation...

For those arbitrary types, pydantic validates by just checking isinstance(value, type). However Element instances created in one module will sometimes fail an isinstance check in another! I'm still not sure why this happens. I did observe that when the problem would occur, the imported Element classes in the two different modules were actually different obects with different id() values! I suspect XMLAnnotation creation won't be that popular, so I'm not concerned about leaving this one untyped. Maybe we could type it as Element but disable the pydantic runtime validation? Is that possible? That would provide nicer self-documentation at least.

one that will probably never work

Right, but the lack of support for non-positive years in datetime.datetime doesn't seem like a problem in practice since the earliest representable date still predates the invention of the (analog) microscope by 16 centuries. :trollface:

@tlambert03
Copy link
Owner

It was part of my experiment in typing XMLAnnotation.value as ElementTree.Element, but since that failed for other reasons you won't see it in the committed code.

ah! I see... sorry i misunderstood. Yeah I've seen that pydantic config setting before.

I did observe that when the problem would occur, the imported Element classes in the two different modules were actually different objects with different id() values!

wow. that's strange... might they be doing something dynamic on import?

doesn't seem like a problem in practice since the earliest representable date still predates the invention of the (analog) microscope by 16 centuries. :trollface:

lol

ok... thanks for this!

@tlambert03 tlambert03 merged commit 96c98fd into tlambert03:master Jul 24, 2020
@jmuhlich jmuhlich deleted the annotations branch July 24, 2020 19:19
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.

3 participants