Implement UnionEle.elements
#40
Conversation
Deduplicate logic with `BeamLine` for allowed types and (de)serialization.
|
|
||
|
|
||
| class BaseElement(BaseModel): | ||
| class BaseElement(BaseModel, validate_assignment=True): |
There was a problem hiding this comment.
Sufficient to define validate_assignment=True once -- in our BaseElement.
|
What is the meaning of the word "mixin"? I searched it in the PALS documentation and the search didn't return any result. I understand that it may be the squash of "mix in", but I wonder if there's a better word that does match the PALS documentation. |
src/pals/kinds/UnionEle.py
Outdated
|
|
||
| @model_validator(mode="before") | ||
| @classmethod | ||
| def unpack_yaml_structure(cls, data): |
There was a problem hiding this comment.
If this model validator is format-agnostic maybe we should not call it unpack_yaml_structure.
There was a problem hiding this comment.
We could rename to json, which is the default model for pedantic, since this overwrites/enhances a pedantic internal unpacking routine.
A more generic name might be obfuscating/hard to grasp.
There was a problem hiding this comment.
I don't agree with this - if the data that the function receives as input is format-agnostic, I don't see why the function should be called unpack_yaml_structure or unpack_json_structure.
I think a function name that is format-agnostic would be fine and less misleading, e.g., unpack_data, unpack_structure, unpack_union_data, unpack_union_structure, unpack_data_structure, etc.
This said, I don't have veto power, so I will approve the changes.
|
The changes to the tests look fine to me, though I don't know if they cover everything that this PR added. |
https://en.wikipedia.org/wiki/Mixin We use the same name in ImpactX for mixin classes. They can be base classes, but don't have to. They are a standard programming technique for implementation details, not PALS objects. |
|
In my opinion, it would makes sense to finalize this after the union element has been clarified on the PALS end. See originally pals-project/pals#86, now moved to pals-project/pals#100. If you believe otherwise, of course feel free to merge. |
|
The primary thing that will change is the class name, thus I would go ahead and merge the logic already. We have versions on PALS-Python to iterate and break things. |
|
Okay. Based on this morning's discussion it seemed to me that there was more debate about what a union element is than just the class name, but probably I misread the room and/or misinterpreted the comments. |
Good to explain this pydantic term a little
|
Yes, I think for now we should expect that there will be some kind of element like Union/Girder, that behaves so far 99% like a |
2 participants
UnionEle: Deduplicate logic withBeamLinefor allowed types and (de)serialization. Close Implement Union Element #39