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
Simplify graph viewer rendering logic #8227
Simplify graph viewer rendering logic #8227
Changes from 14 commits
e62c933
7cc8a42
cec7395
48957e6
c189ef5
145a1a4
45a2dc7
22b5489
2a1535c
226804c
f8429ae
f50d698
19902a3
96927da
0cc4fec
2de4d1c
1874a67
dedd199
8ff48a2
86bb623
7fcbb1a
db3a546
98c9fc8
96f7a53
f8fd175
9df6694
834fb2f
9a5f261
8d8056a
7a4ad6d
5c00d50
95e12e3
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.
Docstring these! I would not expect a reader to understand what an implicit node is (I don't)
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've added doc comments, maybe this makes it clearer. Because
Node::Implicit
only has a very limited subset of information, I chose to introduce the enum at the top level to avoid any confusion. This is also the reason wy I opted against anenum NodeCategory { Explicit, Implicit }
.We could also introduce a mixture of both approaches, but that would introduce another level of indirection, which I think can be avoided by a little redundancy.
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.
Maybe it would make more sense with a
enum NodeCategory { Implicit, Explicit }
andstruct Node { category: NodeCategory, id: … }
?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.
See above.