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
Introducing PartialOrder #288
base: main
Are you sure you want to change the base?
Introducing PartialOrder #288
Changes from 20 commits
0aca75d
da4fe74
b7d8835
1c92c81
031e3a9
87c1a67
6824321
ed26c84
b79cb47
f44387f
e00f63c
e5a685a
164a68d
a88c502
ccbca4b
55ee2e8
2adbd02
d89ee63
93f3952
9651875
f05da6f
8c8d637
d4941f5
744ccba
4fcb952
829926c
ab10364
23ae475
07268fd
a5341ad
d28fb49
31b963a
a9e96ab
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.
Here I have a big question: why not inherit from the compas
Graph
instead? Otherwise, we will definitely duplicate features. As an example, I would almost immediately need to be able to turn a plan back and forth from a NetworkX DiGraph, and that works withGraph
but not if the object inherits directly fromDatastructure
. The alternative would be containment of aGraph
, but I am not sure that'd be very clean.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.
Unsure about the typing here, and how sphinx wants it...
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 the pair
plan_action
andappend_action
not very intuitive as being a pair of related functions.Why not merge them both into
add_action
, if thedependency_ids
param is none/empty, then do the appending logic?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 wanted to make a bigger distinction between
dependency_ids=None
anddependency_ids=[]
because they result in very different behavior and I can foresee that being the source of annoying bugs for the user.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.
Do we want these to work in Ironpython?
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.