-
Notifications
You must be signed in to change notification settings - Fork 146
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
Change Graph.__getitem__ to shorthand for get_cursor #380
base: develop
Are you sure you want to change the base?
Conversation
Trying to think about implications first but the api looks indeed nicer the way you propose it. |
Tests are broken, can you have a look ? |
Looks like tests are passing now, there was one test that was using the |
Did a couple more fixes and it looks like all tests are passing now. Let me know if I should check on anything else. |
I'm starting to test your branch on some transformations I run to try to find BC breaks. Would you be kind enough to add a few documentation patches to explain the [] operator usage ? I think the doc should explain it calls the lower-level API "get_cursor" but encourage usage of [] instead of the method call. |
sure. I'll take a look and see where a good place would be to update it. |
I would add my contribution, because I was actually building my own pipeline library but I have better to contribute to your, as bonobo is more far than I am . This is how I implemented my graph builder
|
currently
Graph.__getitem__()
is implemented as:which makes sense... but it only saves about 6 characters of typing
graph[a]
vsgraph.nodes[a]
. However... one thing that's kind of clunky is:If
Graph.__getitem__()
was reimplemented as a shorthand forGraph.get_cursor()
, the above example could be reduced to:which I think is much much cleaner and more intuitive. It saves time typing
get_cursor()
(which users will be doing much more often than retrieving nodes by their numerical index) and is also easier to read. It may even be possible this way to hide the existence of theGraphCursor
class from top-level API's entirely.Obviously this would be a breaking change, but it would be a very simple matter to regex find and replace
graph\[(.+)\]
withgraph\.nodes\[$1\]
.As a bonus, this could also eliminate the need for the
Graph.orphan()
method, since one could simply callgraph[None]
to achieve the same functionality.