Use __str__ to replaced print_hierarchy
#1314
Conversation
|
this looks great, but curious what's @tomvanmele @Licini take on str vs repr: could repr just invoke str? |
|
the point of |
|
right, repr(oduction) its a neat feature, so repr can echo str when reconstruction isn't viable |
|
|
||
| if max_depth is not None and depth > max_depth: | ||
| return | ||
|
|
||
| connector = "└── " if last else "├── " | ||
| print("{}{}{}".format(prefix, connector, node)) | ||
| hierarchy["string"] += "{}{}{}\n".format(prefix, connector, node) |
There was a problem hiding this comment.
why do you pass around a dictionary with a single, hard-coded key?
There was a problem hiding this comment.
This was to make the string a mutable object (like a pointer). In python 3 i can simply use nonlocal and just say hierarchy_string, but that option is not available for 2.7. Maybe now looks better with the list?
|
@Licini sorry to ping, I'd just like to merge BlockResearchGroup/compas_model#31 soon |
|
@Licini would indeed be good to finish this... |
|
@chenkasirer @gonzalocasas Sorry for late update on this folks. But now I can declare this is done 😂. If you guys give me a go I will merge now |
| str | ||
| The spatial hierarchy of the tree. |
There was a problem hiding this comment.
fwiw, i don't agree with this. why would anyone expect Tree.hierarchy to return a string?
There was a problem hiding this comment.
if the only purpose of this method is to be a helper for __str__, i think it should be named differently and made private.
There was a problem hiding this comment.
How about something more expressive like get_hierarchy_string ? Tho I'm more on the side for still keeping it public because I call this function a lot with different max_depth for very deep trees, I can imagine people might also want to do something like this
There was a problem hiding this comment.
@tomvanmele any comments on this? would like to resolve it
According to @chenkasirer there are some discussion about remove all
printin core library, instead use__str__for this type of functionalities.BlockResearchGroup/compas_model#31 (comment)
I made this change for
Tree, let me know what you guys think.What type of change is this?
Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.CHANGELOG.mdfile in theUnreleasedsection under the most fitting heading (e.g.Added,Changed,Removed).invoke test).invoke lint).compas.datastructures.Mesh.