reflectivity / orsopy

http://www.reflectometry.org/orsopy/
MIT License
0 stars 8 forks source link

Alternative dataclass solution using from_dict method, would close Closes #114 #116 #120

Closed aglavic closed 9 months ago

aglavic commented 9 months ago

I've implemented a version based on @bmaranville 's proposal of using normal dataclasses and a from_dict method to implement our custum functionality. Besides a few fixes for the NeXus implementation this works quite well.

I found an elegant and clear solution for the comment attribute as well as keeping track of Header classes for the NeXus implementation.

With this solution the interface is exactly like a normal dataclass with exception of the added comment and the ability to creat it from the dictionary with additional user attributes. This makes our code more readible as well as extension straight forward.

There are a few other bug-fixes that we should merge in case we choose not to go with this solution.

bmaranville commented 9 months ago

I can work on the nexus fixes if you like

EDIT: it looks like you already fixed the nexus read/write - great!

bmaranville commented 9 months ago

@aglavic I have a completed upgrade of the schema generator ready to go using pydantic v2. We don't need to inherit from pydantic dataclasses anymore, and can remove orsopy.dataclasses or orsopy.dataclass or whatever is left.

Can we chat about this soon before proceeding further?

aglavic commented 9 months ago

@aglavic I have a completed upgrade of the schema generator ready to go using pydantic v2. We don't need to inherit from pydantic dataclasses anymore, and can remove orsopy.dataclasses or orsopy.dataclass or whatever is left.

Great, I guess we can merge that after. Would be nice to get rid of the monkey patching there.

@bmaranville I'm now done with this branch. So we can have a chat when we find the time. Tomorrow in pretty free, for example.

aglavic commented 9 months ago

OK. I'll wait for @andyfaff to have a look at it and if he agrees I'll go ahead.

aglavic commented 9 months ago

@andyfaff do I assume correctly, that you are fine with this merge now?