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
Check generated RBI files for correctness #842
Conversation
update_gem_rbis_strictnesses([], gem_dir: @gem_dir, dsl_dir: @outpath.to_s) | ||
say("") | ||
validate_rbi_files( | ||
command: default_command(:dsl, @requested_constants.join(" ")), |
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 would be better to use the real Thor command here but I believe it would require a lot of refactoring
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 enough.
229791c
to
8446ab7
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.
Nice! Just a couple of comments.
update_gem_rbis_strictnesses([], gem_dir: @gem_dir, dsl_dir: @outpath.to_s) | ||
say("") | ||
validate_rbi_files( | ||
command: default_command(:dsl, @requested_constants.join(" ")), |
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 enough.
8446ab7
to
94699a3
Compare
@@ -48,10 +107,8 @@ def update_gem_rbis_strictnesses(gem_names, gem_dir: DEFAULT_GEM_DIR, dsl_dir: D | |||
files | |||
.uniq | |||
.sort | |||
.select do |file| | |||
name = gem_name_from_rbi_path(file) | |||
file.start_with?(gem_dir) && (gem_names.empty? || gem_names.include?(name)) |
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.
Before the auto fix of the typed-override was only applied on requested gems but it doesn't make sense as a newly generated RBI can produce conflicts in old RBIs so I removed the feature.
@@ -1356,30 +1356,6 @@ def quux(x); end | |||
assert_success_status(result) | |||
end | |||
|
|||
it "must turn the strictness of files with error to false only in asked gems" do |
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.
Relative to the feature removed as explained in https://github.com/Shopify/tapioca/pull/842/files#r835335074.
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 a small note about No errors
instead of No error
, otherwise, LGTM.
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
94699a3
to
7353935
Compare
Motivation
Catches parse errors in generated RBI files and display a helpful error message:
Closes #759.
Implementation
Since we already run
sorbet
on generated files to catch redefinition errors and change the strictness of generated files, I decided to hook on this process to check if we find parse errors (code < 4000) as well.Tests
See automated tests.