-
Notifications
You must be signed in to change notification settings - Fork 42
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
[IO-1463][external] __str__ & __repr__ dunders for Meta objects #694
Conversation
… pulled with folders
"Merge master into branch due to divergent flow"
IO-1463 Write str/repr dunders for objects
For objects such as
Make str wrapper generalisable rather than individual if possible |
- Stage Name: {self._element.name}\n\ | ||
- Stage Type: {self._element.type.value}\n\ | ||
- Stage ID: {self._element.id}" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to do this with dictionary mapping using either f-strings or .format(), but the return values include evaluated expressions. A dictionary mapping would require that all expressions were evaluated for all classes, which isn't possible because only one class is present, meaning attributes to evaluate expressions for the other classes aren't available
So instead I used an if-else structure
- Workflow Name: {self._element.name}\n\ | ||
- Workflow ID: {self._element.id}\n\ | ||
- Connected Dataset ID: {self._element.dataset.id}\n\ | ||
- Conneted Dataset Name: {self._element.dataset.name}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about checking for a disconnected workflow - But it seems that when we create a workflow we raise an error if it's disconnected, so it should never happen here
As a general query on testing - I wrote tests for dataset, stage, and team. These use pre-existing object_meta fixtures As far as I could see, no fixtures of this type were available for workflows or team members yet. I had a go at writing them but started to go down a bit of a rabbit hole. I'd appreciate a quick walk-through on how this might be done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, LGTM, one consider point, no change needed
darwin/future/meta/objects/base.py
Outdated
- Stage ID: {self._element.id}" | ||
|
||
else: | ||
raise Exception(f"Class name {class_name} not found in __str__ method") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be useful to add a pprint
of anything that makes it to this point. I'm on the fence whether this should raise, because the most likely case of this happenning is if we forget to implement a new one. I guess we'd find out quickly, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I nearly didn't include this
Will just pprint anything that reaches it w/out raising
Problem
There's currently no str & repr overrides for meta objects. This prevents users from quickly seeing useful information about these objects
Solution
Changelog
Implemented str & repr overrides for Meta objects.