Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement more detailed
__lt__
for components. #1882base: main
Are you sure you want to change the base?
Implement more detailed
__lt__
for components. #1882Changes from 12 commits
707aa88
35b63a8
6bc7dc1
1265d7c
dbed10f
0fd1632
cca4a14
06f1ec1
37e8f0b
af44807
c65a0c3
9cb98e5
d50b6dc
d1db895
6c891fd
914fe58
88feb80
0b1b6ef
12980cd
a62fd41
acbf2fc
53780e4
2a7e4a1
64d1d09
37e4310
80d57ab
9457b49
6dca45a
4e2234b
48603a4
3ccdc5b
2075f3a
5b601f9
15155be
7733ad8
9b61363
b87c1d1
557a333
0fb4219
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 seems like an okay thing for a shaped component to have to define on their own. Maybe a
DerivedShape
is the only exception.Saying this because we
try
/catch
aNotImplementedError
below so ifComponent.getCircleInnerDiameter
is required for components, we'd be okay in that context.But that could be a larger change that is likely better served outside this PR.
Non-blocking comment. If there's traction I can make a ticket
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.
The assumption for the unshaped component is to return the smallest possible bounding circle outer diameter, i.e., assume the component has a disk shape. It seems consistent with that assumption to assume that the inner diameter is 0.0.
The exception is caught in
__lt__
, but only for the purpose of producing a clearer error message. The error is still raised and the code crashes. So we still need to be able to sortUnshapedComponent
s.