-
Notifications
You must be signed in to change notification settings - Fork 389
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
compute
API changes
#1417
compute
API changes
#1417
Conversation
Adds `return_type` kwarg that can be set to: * 'core' (current default) -- odos return value into a core backend * 'native' -- leaves return value alone, doesn't modify at all * user supplied type -- passed on to internal `odo` call.
@@ -45,10 +45,13 @@ | |||
|
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 need the changes in here?
@llllllllll agreed on your comments, and if we go with this approach, we'll definitely make those improvements. This is just a quick and dirty POC to demonstrate the API I'm thinking of, and how it can be implemented without a major rename or big refactoring. What are your thoughts on the API? |
I like the API. I would like to be able to register new return types like discussed in the other issue but for a first pass this looks great. |
+1, it looks good.
|
@llllllllll will be merging this in the next day or so, last call. |
No other comments, thanks! |
@postelrich @llllllllll here's my proof-of-concept for the
compute()
API changes. It's messy, and some of the imports will have to change, and we'll need some small refactoring, but I like that it doesn't require huge code changes, and fits in with the existingcompute
implementation easily.Example usage:
The default gives back a
list
as one would expect:And the return type can be easily overridden: