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

Fix signature for Kernel.system. #3728

Merged
merged 2 commits into from
Dec 2, 2020
Merged

Conversation

reitermarkus
Copy link
Contributor

@reitermarkus reitermarkus commented Nov 24, 2020

Motivation

Kernel.system has some functionality which is not covered by the current signature at all.

Currently, out of these

system('echo')
system('echo', err: File::NULL)
system('echo', out: :err)
system('echo', 'hello')
system('echo', 'hello', err: File::NULL)
system('echo', 'hello', out: :err)
system(['echo', 'echo'])
system(['echo', 'echo'], 'hello')
system(['echo', 'echo'], out: :err)
system(['echo', 'echo'], 'hello', out: :err)
system(({'VAR' => 'VAL'}), 'echo')
system(({'VAR' => 'VAL'}), 'echo', out: :err)
system(({'VAR' => 'VAL'}), ['echo', 'echo'])
system(({'VAR' => 'VAL'}), ['echo', 'echo'], out: :err)
system(({'VAR' => 'VAL'}), ['echo', 'echo'], 'hello')
system(({'VAR' => 'VAL'}), ['echo', 'echo'], 'hello', out: :err)

only

system('echo')
system('echo', 'hello')

type-check. The fixed signature should cover all of these, however it also accepts some configurations which are wrong, i.e.

system('echo', ['echo', 'echo'])
system(['echo', 'echo'], ['hello', 'hello'])

I'm not quite sure if there is a way to make the second argument depend on the first argument, i.e. only allow [String, String] as the second if the first is T::Hash[String, T.nilable(String)].

Test plan

n/a

@reitermarkus reitermarkus requested a review from a team as a code owner November 24, 2020 08:02
@reitermarkus reitermarkus requested review from elliottt and removed request for a team November 24, 2020 08:02
@elliottt
Copy link
Contributor

Could you add some tests for calling Kernel.system with this new signature in test/testdata/rbi/kernel.rb?

Copy link
Contributor

@elliottt elliottt left a comment

Choose a reason for hiding this comment

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

The signature change makes sense, but adding some tests to make sure that expected uses are possible would be great.

@elliottt
Copy link
Contributor

We have a policy of testing changes to Sorbet against Stripe's codebase before
merging them. I've kicked off a test run for the current PR. When the build
finishes, I'll share with you whether or how it failed. Thanks!

Stripe employees can see the build result here:

https://go/builds/bui_IS5iftBlSiD2Rm

Copy link
Contributor

@elliottt elliottt left a comment

Choose a reason for hiding this comment

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

Thanks!

We should hold off on merging this until the stripe migration PR has also been merged.

@elliottt elliottt merged commit 9a8a9d6 into sorbet:master Dec 2, 2020
@reitermarkus reitermarkus deleted the system branch December 2, 2020 19:11
soam-stripe pushed a commit that referenced this pull request Dec 3, 2020
* Fix signature for `Kernel.system`.

* Add tests for `Kernel.system`.
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