-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Support RBS for aws-sdk-core and service gems #2961
Conversation
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.
In general this looks good. Thank you! I would prefer to release this once we get the service RBS generation ready. When we're ready, we can set the minimum version here (https://github.com/aws/aws-sdk-ruby/blob/version-3/build_tools/services.rb#L13)
gems/aws-sdk-core/CHANGELOG.md
Outdated
@@ -1,6 +1,8 @@ | |||
Unreleased Changes | |||
------------------ | |||
|
|||
* Feature - Add RBS |
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.
Can we be a little more specific about what files are added?
Looks like RBS fails with Jruby 9.3 and 9.4. Can you investigate? |
Thank you for reviewing. Even if this is released, I was dealing with the problem that it could not handle types properly due to conflicts with gem_rbs_collection. See also ruby/rbs#1659
Yes, I'll investigate this issue. |
RBS includes C extension. |
@mullermp |
The code for automated releases uses an internal system, but we can add this task to it. What does including documentation in RBS do? It seems like a lot of duplicated information w/ the existing documentation? |
Since our documentation is code generated, I agree that we should probably only have one location for it (source code), unless there's good reason. |
@alextwoods @mullermp hover with rbs documentation example on vscode: Certainly, adding documentation to RBS leads to double management of documentation and complicates the build tasks. Additionally, the behavior of tools might change in the future. |
That's actually a great feature! I use RubyMine as my editor and it does not do that. I think that the code generated documentation (using docs json), the same documentation we use on client methods and params, etc, can be code generated in rbs too. Then management of this becomes easy. We can probably copy docs manually for the few hand written code. |
What do you mean by documentation can be provided to rbs? I was assuming that it is just inline documentation in the same rbs file. If we are code generating these files, we can add the same documentation that's in the source code. |
RBS has a feature that allows copying existing rdoc comments into RBS files. rule /^rbs:doc:aws-sdk-\w+$/ do |task|
name = task.name.split(':').last
sh("bundle exec rdoc --ri --output tmp gems/#{name}/lib")
sh("bundle exec rbs annotate --dir tmp gems/#{name}/sig")
end |
Alternatively, we might consider adding the following to the build task: diff --git a/tasks/build.rake b/tasks/build.rake
index 8767374fc3..06bac3aa20 100644
--- a/tasks/build.rake
+++ b/tasks/build.rake
@@ -22,6 +22,12 @@ rule /^build:aws-sdk-\w+$/ do |task|
)
writer = BuildTools::FileWriter.new(directory: "#{$GEMS_DIR}/#{service.gem_name}")
writer.write_files(files)
+
+ if ENV['GEN_RBS'] && ENV['GEN_RBS'].split(',').include?(identifier)
+ name = task.name.split(':').last
+ sh("bundle exec rdoc --ri --output tmp gems/#{name}/lib")
+ sh("bundle exec rbs annotate --dir tmp gems/#{name}/sig")
+ end
end |
That makes sense! For doc generation, we use I think the biggest concern we have is gem size, especially for S3. The generated documentation is very large. We may want to consider truncating it such that it links to public doc pages instead. |
As of now, RBS does not have the functionality to read cache of YARD When calculating the gem size, for |
It would be the workaround for now. Does it help if |
I'm not sure if an option to delete installed RBS is appropriate or not.. I think the gem size increase is a concern. Could we move forward with not including docs for now and consider an enhancement to add them when we've put more thought into the correct approach and/or when yard is supported? I think at worst case, we can code generate the links to the documentation of each Client operation. i.e. insert operation name |
I'm not sure if this is possible, but it would be nice if doc comments on RBS (or RBS tooling) could support a magic tag like |
Got it. 👍
It should be possible. Something like https://shopify.github.io/ruby-lsp/RubyLsp/Requests/DocumentLink.html in Ruby LSP in RBS too would help... |
26667d9
to
4ded452
Compare
@soutaro @mullermp @alextwoods
|
4ded452
to
cde81ed
Compare
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.
In my environment, I have been able to generate valid RBS for the following gems.
It's okay to release aws-sdk-core
in its current PR.
- aws-sdk-s3
- aws-sdk-sqs
- aws-sdk-lambda
- aws-sdk-ec2
- aws-sdk-cloudfront
- aws-sdk-kms
- aws-sdk-firehose
- aws-sdk-sns
- aws-sdk-ssm
# RBS does not support Delegator. | ||
# the behavior is mimicked `Seahorse::Client::Response` as much as possible. | ||
# --> | ||
module DelegatorMethodsForResponse[DATA] |
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 module does not actually exist.
In RBS, it's not possible to represent the behavior of Delegator, so we can't make the return value of the operations method group Seahorse::Client::Response
.
Instead, we define a non-existent module called DelegatorMethodsForResponse
to represent the methods defined in Seahorse::Client::Response
.
By including DelegatorMethodsForResponse
in the data class, we can represent the actual behavior.
example:
module Aws
module Lambda
module Types
class DestinationConfig
include Seahorse::Client::DelegatorMethodsForResponse[DestinationConfig]
attr_accessor on_success: OnSuccess
attr_accessor on_failure: OnFailure
end
end
end
end
Moreover, DelegatorMethodsForResponse
cannot be an interface. An interface does not allow for duplicate methods. The existence of Aws::Lambda::DestinationConfig#on_success
would result in a method duplication, which is why it cannot be handled as an interface.
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 looks great! Thank you for all the hard work. :D
We should keep this PR until the service gem changes are ready. It is easier for our release system if we do them together.
@@ -10,7 +10,7 @@ Gem::Specification.new do |spec| | |||
spec.homepage = 'https://github.com/aws/aws-sdk-ruby' | |||
spec.license = 'Apache-2.0' | |||
spec.require_paths = ['lib'] | |||
spec.files = Dir['LICENSE.txt', 'CHANGELOG.md', 'VERSION', 'lib/**/*.rb', 'ca-bundle.crt'] | |||
spec.files = Dir['LICENSE.txt', 'CHANGELOG.md', 'VERSION', 'lib/**/*.rb', 'ca-bundle.crt', 'sig/**/*.rbs'] |
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.
Small nit - place this after lib/
And also add CI for RBS
cde81ed
to
9f7a40b
Compare
@mullermp @alextwoods |
Thank you for all of your hard work. I think this is just about ready for launch. Let's target next week. Please be advised if there are any customer issues or bugs - we will try our best to fix them, but we may need help from the RBS experts :) |
I'm proud to have been entrusted with such an important work 👍 |
I am typing up a blog post to be posted alongside merging this change. Could you please assist in writing a minimal case for validating rbs types when using the SDK? I tried the following:
but |
I was also able to get type information in RubyMine but not vs code. Do you have any setup information for displaying type mismatch in vs code? |
SteepThe most common way to read RBS with steep is to use https://github.com/ruby/rbs/blob/master/docs/collection.md The setup in project when using
This will automatically read the gem signatures described in the Gemfile.lock. target :test do
check "test.rb"
end Sample codeUnfortunately, it is currently difficult to make the following code a type error. client = Aws::S3::Client.new(region: 123) The main reason is that the actual aws-sdk implementation does not use the keyword argument. # record pattern
client = Aws::S3::Client.new({region: 123})
# hash pattern
config = {region: 123}
client = Aws::S3::Client.new(config) The record pattern does not currently handle optional keys (implementation is in progress). The hash pattern only allows key and value types to be specified, so per-key checks are not possible. Therefore, many method arguments are resolved with How about an example using Resource instead? resource = Aws::S3::Resource.new
resource.buckets.each do |bucket|
bucket.objects.limit(50).each do |obj|
puts obj.get.etag # String
end
end |
As a side note, rbs v3.4 or higher is required to load gem signatures. |
Thanks for your response on this. RubyMine correctly validates this code (and gives an incompatible types error):
It however validates the record and hash pattern as correct. RE: Steep - I'm looking for a way to have RE: VS Code - what plugins / extensions are you using to make that work? |
Sounds good news! I will try RubyMine too. For vscode userIf you want to use steep with vscode, you need to install the following extension. https://marketplace.visualstudio.com/items?itemName=soutaro.steep-vscode For user without GemfileIf you do not use Gemfile, specify the library to load as follows. # Steepfile
target :lib do
check "test.rb"
library "aws-sdk-core"
library "aws-sdk-s3"
end For developerIf it is for development, I have prepared sample code. Please check it. https://github.com/ksss/aws-sdk-ruby/tree/steep-sample/gems/aws-sdk-s3
|
Thank you for providing some help in this. I'm running into issues with the Steep plugin -
I'm inclined to skip this example for now :( I think the important part of the announcement is that signatures will be available. |
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.
I think this is good to ship!
We will monitor our release tomorrow and we can do a sanity check with the published gems. |
I appreciate the support from the aws-sdk-ruby team ✨ |
Is there anything I can help you? Looks like it's an issue of ruby interpreter loading (or maybe |
Blog post here! https://aws.amazon.com/blogs/developer/announcing-rbs-support-for-aws-sdk-for-ruby-v3/ @ksss Thank you for your work @soutaro Yes, I'll get back to you on trying this, currently working on something else this week. |
@mullermp Nice work! |
#2950
I have added RBS files for aws-sdk-core.
There is no impact on the actual Ruby code.
As a test for RBS, I added CI to run the
rbs validate
command. This test ensures that at a minimum, the RBS can be loaded.