Conversation
|
|
||
| from gemd.entity.dict_serializable import DictSerializable | ||
| from gemd.entity.case_insensitive_dict import CaseInsensitiveDict | ||
| from gemd.entity.link_by_uid import LinkByUID |
There was a problem hiding this comment.
This unfortunately opens the door for circular dependencies, but I had a pickle of a time getting -> 'LinkByUID' to pass flake8.
There was a problem hiding this comment.
You can have flake8 ignore the line. We shouldn't let flake8 prevent us from doing the right thing (and circular dependencies in python have a way of biting you unexpectedly, in my experience).
| if len(self.uids) == 0: | ||
| raise ValueError(f"{type(self)} {self.name} does not have any uids.") | ||
|
|
||
| if scope is None or allow_fallback and scope not in self.uids: |
There was a problem hiding this comment.
I suggest using parentheses to indicate how these logical units are grouped, since (A | B) & C is not the same as A | (B & C).
| """ | ||
| def _make_index(_obj: BaseEntity): | ||
| return (((scope, _obj.uids[scope]), _obj) for scope in _obj.uids) | ||
| return ((LinkByUID(scope=scope, id=_obj.uids[scope]), _obj) for scope in _obj.uids) |
There was a problem hiding this comment.
Does this have any consequences besides the modification to _substitute?
There was a problem hiding this comment.
Apparently DE has built tooling that uses the make_index & tuple representation. With the eq changes to LinkByUID, we should have a drop-in replacement -- I've reached out to DE to verify. I've verified with the citrine-python test suite.
The alternative would be to remove the tuple/LinkByUID equality and have both types of objects serve as indices.
Specially formatted tuples are generally considered bad practice to my understanding.
There was a problem hiding this comment.
Verified that the DE tooling that uses make_index continues to pass tests with these changes.
bfolie
left a comment
bfolie
left a comment
There was a problem hiding this comment.
Why can this not be done with the LinkByUID.from_entity method?
They have similar behavior, yes. The only difference at present is However:
|
2 participants
This PR adds a
to_linkmethod to Base Entities and propagates some benefits from that around the library.to_linkgenerates a Link By UID from the uids present in a Base Entity. If there are no uids, it raises a ValueError. If noscopeto specified, it randomly selects one. If the chosenscopeis not present, depending onallow_fallbackit will either raise a ValueError or return a Link By UID with random scope.One might want to add an autopopulate flag as well, but there was no immediate need for it and not including such functionality is more consistent with the library in general.
substitute_linksingemd.utilwas modified to use the new method. Thenative_uidargument for substitute_links has been deprecated, in favor of the more library-standardscope. It also has had theallow_fallbackargument added, with a default that maintains the existing function behavior.__eq__ingemd.entity.link_by_uidwas modified so that a (scope, id) tuple is equal to a similar Link By UID andmake_indexingemd.utilwas modified to take advantage of this behavior.