-
-
Notifications
You must be signed in to change notification settings - Fork 3
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
Sorbet support (initial) #3
base: master
Are you sure you want to change the base?
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.
Thanks!
Amazing work, left some comments/questions, but I think we'll be ready to merge it soon.
Also, can you please rebase on top of master? I fixed the RBS issues making CI fail.
require "sorbet-runtime" | ||
|
||
# Let methods with sig receive double/instance_double arguments | ||
T::Configuration.call_validation_error_handler = lambda do |signature, opts| |
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 reminds me this RSpec Sorbet integration: https://github.com/samuelgiles/rspec-sorbet/blob/4d0e6479453b3b4ef36d2d6a2df8282ae559368b/lib/rspec/sorbet/doubles.rb#L104
Are we doing something similar here? Maybe, we can rely on the rspec-sorbet
gem? (That's something we can consider in the future)
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 uses a similar approach and calls the same method T::Configuration.call_validation_error_handler
from Sorbet.
The first problem I see here is that they may not work together at the moment. The reason for that is this line:
T::Configuration.send(:call_validation_error_handler_default, signature, opts)
Most probably, it will not call the handler defined in the gem rspec-sorbet
I will try to reproduce the error and write a comment about the result below. Anyway, it should be easy to fix. RSpec Sorbet uses a correct approach to override this method.
It should be possible to integrate RSpec Sorbet gem if this is needed I guess.
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 fixed this issue in another branch: link to the commit
I will add this change to the master
branch if you don't mind.
README.md
Outdated
Gem `sorbet-runtime` does not load signatures for stdlib types (Integer, String, etc...) into runtime. | ||
Checking types for Integer, String, etc is only available through `rbs typecheck` command which uses [custom ruby binary](https://github.com/sorbet/sorbet/blob/master/docs/running-compiled-code.md). | ||
|
||
Therefore, you should consider changing `raise_on_missing_types` to `false` if you use Sorbet. |
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.
Maybe, we can somehow distinguish stdlib types from custom types, so we can ignore only missing stdlib types? Any ideas?
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.
Thanks for the suggestion, it should be easy to implement.
# sorbet.rb
def sig_is_missing(method_call, raise_on_missing)
return unless raise_on_missing
return unless method_call.receiver_class < T::Sig # By adding this line to check if the class uses signatures
raise MissingSignature, RAISE_ON_MISSING_MESSAGE
end
I should also mention that I missed an important part of the problem here and I should update the readme to include this information.
If a signature is described in a .rb
file, it will be used by sorbet-runtime
and type checking will be available. One of the gems that is using sorbet signatures is ShopifyAPI for example.
However, many signatures are declared inside .rbi
files, like 1) signatures for stdlib and core types and 2) signatures for most libraries including rails. Unfortunately, these types cannot be loaded into runtime at the moment. Therefore, it's not possible to type check their mocks yet.
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 also updated readme to make the explanation more understandable.
link to the comment
I can add this commit to the master
branch too.
method_name = method_call.method_name | ||
mocked_obj = method_call.mocked_obj | ||
return_value = method_call.return_value | ||
mocked_obj.define_singleton_method(method_name) { |*args, &block| return_value } |
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.
Defining a singleton method on a mocked object might cause a problem. It's better to either .dup
or create a new empty object for the purpose of holding this method.
@load_dirs = Array(load_dirs) | ||
end | ||
|
||
def typecheck!(method_call, raise_on_missing: false) |
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.
There's a room for improvement: the variable method_call
is passed too many times.
Therefore, it should be better to extract these functions into a class or a structure MethodCheck
, where method_call
(and mocked_obj
, arguments
, method_name
, etc...) will be available as instance variables.
Co-authored-by: Vladimir Dementyev <[email protected]>
Co-authored-by: Vladimir Dementyev <[email protected]>
spec/support/integration_helpers.rb
Outdated
ROOT_DIR = File.expand_path(".") | ||
RSPEC_STUB = File.join(ROOT_DIR, "bin/rspec") | ||
|
||
def run_rspec(path, chdir: nil, success: true, env: {}, options: "") | ||
command = "#{RUBY_RUNNER} #{RSPEC_STUB} #{options} #{path}_fixture.rb" | ||
command = "#{RUBY_RUNNER} #{RSPEC_STUB} #{options} spec/fixtures/rspec/#{path}_fixture.rb" | ||
output, err, status = Open3.capture3( | ||
env, | ||
command, | ||
chdir: chdir || File.expand_path("../../fixtures/rspec", __FILE__) |
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.
What's the purpose of these changes?
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 made these changes because I wanted to run the command
right from the terminal and to make the command
variable look "prettier".
This is how it used to be:
bundle exec ruby /home/yu/projects/mock-suey/spec/support/../../bin/rspec double_fixture.rb
which runs from spec/fixtures/rspec
. There's a part ../../
, which I removed.
This is how it looks now:
bundle exec ruby /home/yu/projects/mock-suey/bin/rspec spec/fixtures/rspec/stored_mocked_calls_fixture.rb
It runs from project root.
@@ -19,7 +19,7 @@ def initialize(contract, msg) | |||
def captured_calls_message(calls) | |||
calls.map do |call| | |||
contract.args_pattern.map.with_index do |arg, i| | |||
(ANYTHING == arg) ? "_" : call.arguments[i].inspect | |||
(arg == ANYTHING) ? "_" : call.arguments[i].inspect |
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 out of curiosity: why you swapped these two arguments of ==
operator?
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 reversed these arguments because rubocop showed me a warning Style/YodaCondition
. link to the rule.
It looks a bit more logical to put the arg
first: "argument equals constant"
vs "constant equals argument"
.
Anyway, I don't mind both kinds of comparations. It is easy to read both of them.
Implementation proposal
Initial setup details
sorbet
toTYPE_CHECKERS
incore.rb
lib/type_check/ruby.rb
as a template for the new checkerlib/type_check/sorbet.rb
(and tests too)Sorbet library integration details
Option 1: Use Sorbet methods inside
typecheck!
validate_call
and related methods insidetypecheck!
Looking for the signature of the original method
Typecheck 2: CallValidation
It is possible to check types of incoming arguments, which I am also going to implement using
CallValidation
Useful Mock Suey methods
I include methods from Mock Suey that might be used during this feature development, or at least their understanding is necessary for me to write the code. They also might be helpful during code review.
Useful Sorbet methods
I am going to use sorbet-runtime as it includes signatures and typechecker.
sig
methodmodule
Methods
which contains the most important source code for me.Other useful things:
caller_location
sorbet configuration
method_added