Skip to content
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

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

oatmealdealer
Copy link

currently Graph.__getitem__() is implemented as:

def __getitem__(self, key):
    return self.nodes[key]

which makes sense... but it only saves about 6 characters of typing graph[a] vs graph.nodes[a]. However... one thing that's kind of clunky is:

graph >> a >> b >> c
graph.get_cursor(a) >> d >> e    # adds nodes that receive data from a

If Graph.__getitem__() was reimplemented as a shorthand for Graph.get_cursor(), the above example could be reduced to:

graph >> a >> b >> c
graph[a] >> d >> e

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 the GraphCursor 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\[(.+)\] with graph\.nodes\[$1\].

As a bonus, this could also eliminate the need for the Graph.orphan() method, since one could simply call graph[None] to achieve the same functionality.

@hartym
Copy link
Member

hartym commented Apr 29, 2020

Trying to think about implications first but the api looks indeed nicer the way you propose it.

@hartym
Copy link
Member

hartym commented Apr 30, 2020

Tests are broken, can you have a look ?

@oatmealdealer
Copy link
Author

Looks like tests are passing now, there was one test that was using the __getitem__ behavior in its assertions that needed to be updated.

@oatmealdealer
Copy link
Author

Did a couple more fixes and it looks like all tests are passing now. Let me know if I should check on anything else.

@hartym
Copy link
Member

hartym commented May 5, 2020

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.
Thanks

@oatmealdealer
Copy link
Author

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.
Thanks

sure. I'll take a look and see where a good place would be to update it.

@MChrys
Copy link

MChrys commented May 18, 2020

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

# parser  & add edges examples
#    T = graph()
#    T >> s1 >>  (s2 >> s5) + s3 
#>>>      s1
#>>>     /  \
#>>>    s2  s3
#>>>   /     |
#>>>  s5     |
#>>>  |      |
#>>>  O      O
#
#    T[[:s5>]] >> s6
#
#>>>      s1
#>>>     /  \
#>>>    s2  s3
#>>>   /     |
#>>>  s5     |
#>>>  |      |
#>>>  s6     |
#>>>  |      |
#>>>  O      O
# 
#OR 
#    T |  s5 >> s6
#
#>>>      s1
#>>>     /  \
#>>>    s2  s3
#>>>   /     |
#>>>  s5     |
#>>>  |      |
#>>>  s6     |
#>>>  |      |
#>>>  O      O
#
#   a = T[:s5].clone()
#   a
#>>>      s1
#>>>     /  
#>>>    s2  
#>>>   /     
#>>>  s5     
#>>>  |      
#>>>  O  
#Try pipe fast in notebook with Input test
#   T[[:s5>]].test(Input)
#
#
# 1 er : T.start >> s1 >>  (s2 >> s5) + s3  | (s2 + s3) << s7 
#
#>>>      s1
#>>>     /  \
#>>>    s2  s3
#>>>   / \ / |
#>>>  s5 s7  |
#>>>  |   |  |
#>>>  O   O  O
#start property clean the graph for rebuild it fast in notebook
# a = T[[:s5],[:s7]]
#>>>a
#>>>      s1
#>>>     /  
#>>>    s2 
#>>>   /  \ 
#>>>  s5  s7  
#>>>  |    |  
#>>>  O    O  

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants