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

Improve DelayedCalculation Functions #26

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

Conversation

seyyah
Copy link

@seyyah seyyah commented Jul 6, 2019

I added new functionalities for the DelayedCalculation

  • keys
  • available_resources
  • process_resources

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

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.

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.

# method: :post
}.merge(options)

puts options
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

Copy link
Author

Choose a reason for hiding this comment

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

Ohh I'm sorry.

Copy link
Author

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

@marten marten marked this pull request as draft June 6, 2024 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants