Skip RBS rewriter when file does not contain RBS syntax#917
Conversation
dejmedus
force-pushed
the
jb-rbs-marker-gaurd
branch
from
82e90b7 to
07a9394
Compare
| RB | ||
| end | ||
|
|
||
| def test_should_rewrite_returns_true_for_supported_typed_sigils |
There was a problem hiding this comment.
Two more cases to test:
-
Don't trigger if
# typed: trueexists later in the file. This is unlikely, but it's a performance improvement to ensure we're not scanning through the whole file -
Trigger if there's multiple magic comments, but
typedisn't first:# frozen_string_literal: true # typed: true
There was a problem hiding this comment.
> Don't trigger if # typed: true exists later in the file
Good call! I updated to only check for the typed marker in the magic comments block
Refactored to instead use strictness_in_content and valid_strictness checks from /sorbet/sigils.rb
dejmedus
force-pushed
the
jb-rbs-marker-gaurd
branch
from
07a9394 to
47118ab
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
dejmedus
force-pushed
the
jb-rbs-marker-gaurd
branch
2 times, most recently
from
cf00c3f to
d3125ac
Compare
| next if new_contents == contents | ||
|
|
||
| File.write(file, new_contents) | ||
| transformed_count += 1 |
There was a problem hiding this comment.
This will only count changed files in the Translated signatures in <x> files. message, but I'm happy to drop the commit if we want to leave as is
dejmedus
force-pushed
the
jb-rbs-marker-gaurd
branch
from
d3125ac to
f25aa67
Compare
paracycle
left a comment
paracycle
left a comment
There was a problem hiding this comment.
Shouldn't all this logic be encapsulated inside the RBSCommentsToSorbetSigs class, which already stores the ruby_contents as an ivar, and wouldn't need to pass it to a should_rewrite? method. The downside is just an extra object allocation only to return the same contents, but I think that's fine.
amomchilov
left a comment
amomchilov
left a comment
There was a problem hiding this comment.
Few small points, but it's shaping up!
| end | ||
| end | ||
|
|
||
| Spoom::Sorbet::Translate::RBSCommentsToSorbetSigs::RBS_ANNOTATION_MARKERS = T.let(T.unsafe(nil), Array) |
There was a problem hiding this comment.
Let's mark these with private_constant where they're defined, so we don't expose them to the public.
When a file is not typed or contains no RBS comment syntax, we can skip running the RBS rewriter on it Co-authored-by: Matt Kubej <matt.kubej@shopify.com>
dejmedus
force-pushed
the
jb-rbs-marker-gaurd
branch
from
f25aa67 to
8244d5a
Compare
amomchilov
left a comment
amomchilov
left a comment
There was a problem hiding this comment.
LGTM pending the unresolved comments
| end | ||
| end | ||
|
|
||
| def test_contains_rbs_syntax_returns_true_when_typed_sigils_follow_magic_comments |
There was a problem hiding this comment.
| def test_contains_rbs_syntax_returns_true_when_typed_sigils_follow_magic_comments | |
| def test_contains_rbs_syntax_returns_true_when_typed_sigil_is_after_other_magic_comments |
| RB | ||
| end | ||
|
|
||
| def test_contains_rbs_syntax_returns_false_for_unrelated_yard_tags |
| end | ||
|
|
||
| #: (String ruby_contents, file: String, ?max_line_length: Integer?) -> String | ||
| def rewrite(ruby_contents, file:, max_line_length: nil) |
There was a problem hiding this comment.
| def rewrite(ruby_contents, file:, max_line_length: nil) | |
| def rewrite_if_needed(ruby_contents, file:, max_line_length: nil) |
or something like this
|
|
||
| def test_translate_to_rbi_method_sigs | ||
| contents = <<~RB | ||
| # typed: true |
There was a problem hiding this comment.
Can we instead call the underlying new.rewrite instead so we don't have to rewrite all the tests?
5 participants
If a file does not contain typed file markers (ex
# typed: true) or RBS syntax we can skip attempting to translate it. If a file has not been rewritten we can exclude it from the count of translated files