-
Notifications
You must be signed in to change notification settings - Fork 7
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
Improve DelayedCalculation Functions #26
base: master
Are you sure you want to change the base?
Conversation
@@ -107,7 +107,8 @@ def request_options(data, format) | |||
end | |||
|
|||
def function_url(package, function, user = :system, github_remote = false, format = nil) | |||
"#{package_url(package, user, github_remote)}/R/#{function}/#{format.to_s}" | |||
"#{package_url(package, user, github_remote)}/#{function}/#{format.to_s}" |
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.
Changing the url here.
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.
Yes. I want to generalize the function_url.
For example
- GET https://cloud.opencpu.org/ocpu/library/MASS/data/Boston/json
- GET https://cloud.opencpu.org/ocpu/library/MASS/scripts/
- POST https://cloud.opencpu.org/ocpu/library/MASS/scripts/ch01.R
📋 Reference
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.
Problem is that this will break everyones code. so the mayor version would have to be updated. Feels like the other calls might be better as different endpoints in client? since they are not a function calls and will also not usually work the same way? Can scripts return multiple resources? Also, prefixing all function calls with R/
feels like what a wrapper is there for.
process_resource @available_resources[key].to_s | ||
end | ||
|
||
def process_resources(key, options = {}) |
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.
This needs some comments, some text in the readme and some specs. So it's a nested resources type thing?
There also also a lot of duplication, especially in all the private methods below. We should either reuse the client here to make the request or separate the logic into a separate file.
(Also, I think we can use named parameters now instead of an options hash. 5 years ago, we were asked to support 1.9, but now no-one should use a ruby version that has been unsupported for 4 years. but this is irrelevant if we reuse the client code)
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.
This needs some comments,
I didn't add any comment because the codes do not contain comments in general.
some text in the readme
I can add usage examples/scenarios in the README.
and some specs.
I'm not very good at testing/specs, but I can still try to add.
So it's a nested resources type thing?
It can certainly be done much better than I have offered.
However, I wanted to share with you the code that I have customized for my own needs and that I have solved.
There also also a lot of duplication, especially in all the private methods below. We should either reuse the client here to make the request or separate the logic into a separate file.
(Also, I think we can use named parameters now instead of an options hash. 5 years ago, we were asked to support 1.9, but now no-one should use a ruby version that has been unsupported for 4 years. but this is irrelevant if we reuse the client code)
If this GEM will be re-designed or written, there will be much to change. Yes the codes repeated and bothers me frankly, I have little time for this job. I offer a work-around solution (saves the days).
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.
Thing is, that we only ever use the execute endpoint and never the prepare one, so we are happy to merge useful features, but we only develop what we need ourselves.
lib/opencpu/delayed_calculation.rb
Outdated
# method: :post | ||
}.merge(options) | ||
|
||
puts options |
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.
remove
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.
Ohh I'm sorry.
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.
New commit for this purpose: 37694bb
I added new functionalities for the DelayedCalculation