-
Notifications
You must be signed in to change notification settings - Fork 40
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
Added newer DefaultTraversal rules to align with V3 sharp connectors #367
base: v3-dev
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3-dev #367 +/- ##
==========================================
+ Coverage 89.67% 89.84% +0.17%
==========================================
Files 123 124 +1
Lines 6685 6729 +44
==========================================
+ Hits 5995 6046 +51
+ Misses 690 683 -7 ☔ View full report in Codecov by Sentry. |
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.
all looks good, just general comments/questions
@@ -0,0 +1,29 @@ | |||
from specklepy.objects.base import Base |
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.
what is the advantage of creating separate variables and methods instead of putting it as properties and @ staticmethods of new class DefaultTraversal?
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.
No reason... generally I haven't bothered with all static classes before in python...
if you'd prefer it I can do just that.
return any(hasattr(x, alias) for alias in DISPLAY_VALUE_PROPERTY_ALIASES) | ||
|
||
|
||
def create_default_traversal_function() -> GraphTraversal: |
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.
a test missing
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.
Yes we are missing a test for this... I'm not sure we really need one since its just a factory function for traversal rules which are tested...
I can add one if you think its important.
Added the
DefaultTraversal
that C# has been enjoying. (added by specklesystems/speckle-sharp#3327)Reminder about what it is, its the new v3 de-facto rules to consume "convertible" objects in the tree.
It has been designed in a way that behaves for all intents and purposes, the same as the traversal logic in v2, but decoupled from the
CanConvertToNative
function that the old v2 function depended on.Its what we should use when consuming objects in our v3 connectors.
Tested with Blender connector (tho I'm not going to update blender until we do a v3)
I haven't done any unit tests for this
DefaultTraversal
, beyond the general ones for theGraphTraversal
, since we don't have any in C#, but perhaps we should...