add EdgeList.__contains__ #754
Conversation
|
Obviously |
| @@ -200,9 +203,9 @@ def __init__(self, start, end, label=''): | |||
| self.start = start | |||
| self.end = end | |||
| self.label = label | |||
| self.key = "%s|%s"%(self.start, self.label) | |||
| self.key = (self.start, self.label) | |||
There was a problem hiding this comment.
These slots are public API of an edge and therefore can't change their value like this.
Same for rkey below.
There was a problem hiding this comment.
So the value of rkey itself is considered public api, not just the value of the expression edges_by_start[rkey]? Darn.
There was a problem hiding this comment.
An Edge is basically like a struct. All member are public and likely being used by user land code..
There was a problem hiding this comment.
Darn, I can't really argue with the fact that people might be relying on this. Is | a valid character in node or edge names? (ie, can I frame this as a bug, in which case breaking reliance of a bug is perhaps more ok)
Or should I just rebase this PR to just be the __contains__ above?
There was a problem hiding this comment.
No, | is not a valid character in node or edge names. So it is safe to be used as a separator.
Yes, please update the PR - and squash the commits on this branch. The __contains__ is certainly the significant of this PR. The slightly easier implementation of __iter__ as well as the set initializations below are more cosmetic but you can keep those.
dirk-thomas
force-pushed
the
indigo-devel
branch
from
ccf6c3a
to
7a3d14d
Compare
|
@ros-pull-request-builder retest this please |
|
Please rebase the patch against current |
Also some small tidy ups
|
Looks like you did a rewrite of published history there! Corrected now |
eric-wieser
changed the title
dirk-thomas
changed the title
|
Thanks for the patch. I have applied a modified version to the kinetic-devel branch: 5de058a Since the |
Part of this pull request was me requesting that it should be, in the same way that If you think it shouldn't be, then that's fine - it's not my call to make. Thanks for the merge! |
|
Personally I do agree with you that cleaning up the API is nice. But based on the amount of code using this API it is not worth "forcing" the people to update it. Sorry. |
Deprecation doesn't ever need to lead to forcing people to update it - my goal was to stop anyone ever using Also, this strikes me as a rather internal API, so I'd be surprised if that much code actually uses it anyway. At any rate, I half-assed this, and if I was going for deprecation should probably have included a |
Python dictionaries support indexing by tuples