-
Notifications
You must be signed in to change notification settings - Fork 7
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
refactor(objects): cnx 687 purge unused classes from objects #333
refactor(objects): cnx 687 purge unused classes from objects #333
Conversation
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.
Checked ArcGIS part - looks good except the Parameters converter, it will look nearly the same for Dictionary object and can be reused for that.
// Revit parameters are sent under the `properties` field as a `Dictionary<string,object?>`. | ||
// This is the same for attributes from other applications. No Speckle objects should have attributes of type `Base`. | ||
// Currently we are not sending any rhino user strings. | ||
// TODO: add support for attributes of type `Dictionary<string,object?>` |
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.
we can just move this part of code lower to the condition "if Dict" rather than if Base. Why removing if the TODO is already done here?
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.
Let's sync on this: archicad, civil3d, revit, and eventually tekla will send some properties/parameters under the properties
field with a dict value - need to implement this in a way in arcgis that can handle diff levels of nesting
…tps://github.com/specklesystems/speckle-sharp-connectors into claire/cnx-687-purge-unused-classes-from-objects
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.
Works for me
Changes to all converters pending this pr in sdk: specklesystems/speckle-sharp-sdk#151 . To be merged after SDK pr is merged.
modelcurves
by sending them as regular geometry instead