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.

Sign up for GitHub

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Improve DelayedCalculation Functions #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0eaf188
Improve DelayedCalculation Functions
seyyah Jul 6, 2019
37694bb
Improve DelayedCalculation Functions: remove test codes
seyyah Jul 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/opencpu/client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the url here.

Copy link
Author

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

  1. https://www.opencpu.org/api.html

Copy link
Contributor

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.

# "#{package_url(package, user, github_remote)}/R/#{function}/#{format.to_s}"
end

def package_url(package, user = :system, github_remote = false)
Expand Down
78 changes: 78 additions & 0 deletions lib/opencpu/delayed_calculation.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ class UnsupportedFormatError < StandardError; end
class ResponseNotAvailableError < StandardError; end

class DelayedCalculation
include Errors
include HTTMultiParty

attr_accessor :location
Expand Down Expand Up @@ -50,8 +51,50 @@ 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 = {})
Copy link
Contributor

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)

Copy link
Author

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

Copy link
Contributor

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.

options = {
user: :system,
format: :json,
data: {}
}.merge(options)

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?
Expand Down Expand Up @@ -83,5 +126,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
2 changes: 1 addition & 1 deletion lib/opencpu/version.rb
View file
Open in desktop
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