-
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?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ class UnsupportedFormatError < StandardError; end | |
class ResponseNotAvailableError < StandardError; end | ||
|
||
class DelayedCalculation | ||
include Errors | ||
include HTTMultiParty | ||
|
||
attr_accessor :location | ||
|
@@ -50,8 +51,53 @@ def info | |
process_resource @available_resources[:info].to_s | ||
end | ||
|
||
def keys | ||
@available_resources.keys | ||
end | ||
|
||
# available_resources(:"R/foo") | ||
def available_resources(key) | ||
raise ResponseNotAvailableError unless @available_resources.has_key?(key) | ||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
I didn't add any comment because the codes do not contain comments in general.
I can add usage examples/scenarios in the README.
I'm not very good at testing/specs, but I can still try to add.
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.
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 commentThe 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. |
||
options = { | ||
user: :system, | ||
format: :json, | ||
data: {} | ||
# method: :post | ||
}.merge(options) | ||
|
||
puts options | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. New commit for this purpose: 37694bb |
||
|
||
raise ResponseNotAvailableError unless @available_resources.has_key?(key) | ||
url = @available_resources[key].to_s | ||
data = options[:data] | ||
format = options[:format] | ||
|
||
process_query url, data, format do |response| | ||
location = response.headers['location'] | ||
resources = response.body.split(/\n/) | ||
OpenCPU::DelayedCalculation.new(location, resources) | ||
end | ||
end | ||
|
||
private | ||
|
||
def process_query(url, data, format, &block) | ||
return fake_response_for(url) if OpenCPU.test_mode? | ||
|
||
response = self.class.post(url, request_options(data, format)) | ||
|
||
case response.code | ||
when 200..201 | ||
return yield(response) | ||
else | ||
fail error_class_for(response.code), "#{response.code}:\n #{response.body}" | ||
end | ||
end | ||
|
||
def process_resource(resource) | ||
response = self.class.get resource | ||
if response.ok? | ||
|
@@ -83,5 +129,40 @@ def key(uri, location) | |
key = :graphics if key =~ /graphics\/\d/ | ||
key.to_sym | ||
end | ||
|
||
def request_options(data, format) | ||
options = { | ||
verify: OpenCPU.configuration.verify_ssl | ||
} | ||
|
||
case format | ||
when :json | ||
options[:body] = data.to_json if data | ||
options[:headers] = {"Content-Type" => 'application/json'} | ||
when :urlencoded | ||
options[:query] = data if data | ||
end | ||
|
||
if OpenCPU.configuration.username && OpenCPU.configuration.password | ||
options[:basic_auth] = { | ||
username: OpenCPU.configuration.username, password: OpenCPU.configuration.password | ||
} | ||
end | ||
options | ||
end | ||
|
||
def error_class_for(response_code) | ||
case response_code | ||
when 403 | ||
AccessDenied | ||
when 400..499 | ||
BadRequest | ||
when 500..599 | ||
InternalServerError | ||
else | ||
OpenCPUError | ||
end | ||
end | ||
end | ||
|
||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
module OpenCPU | ||
MAJOR = 0 | ||
MINOR = 11 | ||
MINOR = 12 | ||
TINY = 0 | ||
VERSION = [MAJOR, MINOR, TINY].join('.') | ||
end |
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
📋 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.