-
Notifications
You must be signed in to change notification settings - Fork 177
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
chore(objects): CNX-8709 suppress warnings in obsolete classes, methods, and fields #3143
Conversation
…com:specklesystems/speckle-sharp into CNX-8709-Objects-Obsolete-remove-or-supress
@@ -32,6 +32,8 @@ int endIndex | |||
IsoStatus = isoStatus; | |||
TrimType = trimType; | |||
IsReversed = reversed; | |||
StartIndex = startIndex; |
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.
Why were we not doing this before?
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'm not sure, @AlanRynne was there a reason you can remember for not setting the StartIndex
and EndIndex
props even though they are arguments passed to the constructor?
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 find no reason why we would ever do this, I'll dig in a bit more after standup.
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 double checked the usages of this constructor and it is definitely a bug/oversight.
This does not affect conversions for the most part, as the start/end vertex index is rarely used... but I'm sure this would fix some odd Brep edge-cases we may have been unaware of.
It will also ensure breps always have the same id (as missing to set start/end vertex would in fact result in a different id for that brep)
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.
This looks good. I added my 2 cents on the BrepTrim discussion, but it's safe to merge!
@@ -32,6 +32,8 @@ int endIndex | |||
IsoStatus = isoStatus; | |||
TrimType = trimType; | |||
IsReversed = reversed; | |||
StartIndex = startIndex; |
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 double checked the usages of this constructor and it is definitely a bug/oversight.
This does not affect conversions for the most part, as the start/end vertex index is rarely used... but I'm sure this would fix some odd Brep edge-cases we may have been unaware of.
It will also ensure breps always have the same id (as missing to set start/end vertex would in fact result in a different id for that brep)
Network
class and related classes are now marked asObsolete
(instead of just the constructor).