-
Notifications
You must be signed in to change notification settings - Fork 245
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
jsii-ruby: low-level ruby client for jsii-runtime #143
Conversation
Implement the low-level API for the jsii-ruby runtime client. This layer starts the jsii-runtime child node.js process and allows interacting it via the stdin/stdout request/response interface. Implemented as a leaky abstraction, so there is no need to implement glue in ruby for each kernel API. Sync overrides are implemented as well. Unit test verifies a common scenario, and specifically verifies synchronous and async virtual overrides. Added minimal "no op" support for ruby pacmak which simply emits the tarballs. This is needed for the initial set of tests. "npm run build" will: - Generate jsii-calc via pacmak (just tarballs for now) - "bundle install" gems - Run "robucop" (ruby linter) - Run "gem build" which builds the ruby gem - Runs unit tests
package_json_path = File.join(File.dirname(__FILE__), '..', 'package.json') | ||
pkg = JSON.parse(File.read(package_json_path)) | ||
|
||
Gem::Specification.new do |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.
Good practice to list the specific versions of ruby
that are supported... Like what syntax level is used?
s.summary = pkg['description'] | ||
s.authors = pkg['author'] | ||
s.files = Dir['lib/**'] | ||
puts s.files |
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.
Nope.
end | ||
end | ||
|
||
define_api(:load) |
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.
Omit the parentheses around arguments here...
return process_error(resp) if resp['error'] | ||
return process_callback(resp) if resp['callback'] | ||
|
||
# nil "ok" means undefined result (or void). |
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.
Not sure this comment belongs here... Maybe document the method instead?
def process_error(resp) | ||
message = resp['error'] | ||
stack = resp['stack'] | ||
message += "\n" + stack unless stack.nil? |
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.
Replace unless stack.nil?
by if stack
.
Also, you should interpolate ("\n#{stack}"
).
end | ||
|
||
def process_callback(resp) | ||
raise JsiiError, 'no callback handler registered with on_callback' if @callback_handler.nil? |
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.
Here unless @callback_handler
.
|
||
begin | ||
result = @callback_handler.call(callback) | ||
rescue StandardError => e |
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.
Synonym of rescue => e
, which is preferred.
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.
Actually, the linter recommends that I specified StandardError
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.
Eowww. Bad linter, bad!
@@ -0,0 +1,6 @@ | |||
module Aws | |||
module Jsii | |||
class JsiiError < StandardError |
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.
But -- is this actually a StandardError?
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.
Why not? Should I use RuntimeError
?
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.
Possibly, yes. But really - do you expect an author of ruby code to "normally" encounter those errors, actually catch them, and recover? If not, it should extend Error
instead.
@@ -0,0 +1,114 @@ | |||
require 'test/unit' |
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.
Would prefer using rspec
.
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.
Since these tests are the same for all runtimes, I rather use same framework type. I also really hate rspec.
assert_equal({ 'value' => 150 }, @client.get(objref: curr, property: 'value')) | ||
|
||
naming = @client.naming(assembly: '@scope/jsii-calc-lib')['naming'] | ||
assert_equal('software.amazon.jsii.tests.calculator.lib', naming['java']['package']) |
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.
Why's the ruby test caring about the java
name of stuff? It shouldn't even care to know what's in java
, and it shouldn't break if java
changes how/where it registers package
.
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.
It's just testing that naming
returns some values. Since this entire layer is one big leak, it doesn't really matter.
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.
"Just testing that X return some values" is not the same as "Testing that X returns some value that contains this very specific bit of information there, which could change under our feet without any consequences"
Follow up on #143 * Add expected ruby version to gemspec * Clean up some non-idiomatic ruby statements
Follow up on #143 * Add expected ruby version to gemspec * Clean up some non-idiomatic ruby statements
Implement the low-level API for the jsii-ruby runtime client.
This layer starts the jsii-runtime child node.js process and allows interacting it via the stdin/stdout request/response interface.
Implemented as a leaky abstraction, so there is no need to implement glue in ruby for each kernel API. Sync overrides are implemented as well.
Unit test verifies a common scenario, and specifically verifies synchronous and async virtual overrides.
Added minimal "no op" support for ruby pacmak which simply emits the tarballs. This is needed for the initial set of tests.
"npm run build" will:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.