Line-matching rewriter#940
Conversation
amomchilov
force-pushed
the
Alex/line-matching
branch
2 times, most recently
from
9aad209 to
df5c587
Compare
amomchilov
force-pushed
the
Alex/line-matching
branch
from
df5c587 to
754641a
Compare
amomchilov
force-pushed
the
Alex/line-matching
branch
from
754641a to
273ef3e
Compare
amomchilov
changed the base branch from
graphite-base/940
to
Alex/Options-object
This stack of pull requests is managed by Graphite. Learn more about stacking. |
amomchilov
force-pushed
the
Alex/Options-object
branch
2 times, most recently
from
eeb32d8 to
d5692f6
Compare
amomchilov
force-pushed
the
Alex/line-matching
branch
from
273ef3e to
4dafa78
Compare
amomchilov
force-pushed
the
Alex/line-matching
branch
from
4dafa78 to
846ea47
Compare
amomchilov
force-pushed
the
Alex/Options-object
branch
from
d5692f6 to
7654edc
Compare
amomchilov
force-pushed
the
Alex/line-matching
branch
from
846ea47 to
0bdd70e
Compare
| #: LineMatchedRBIFormat | ||
| attr_reader :default | ||
| end | ||
| end |
There was a problem hiding this comment.
Will this be used similar to HumanReadableRBIFormat in lib/spoom/sorbet/translate/rbs_comments_to_sorbet_sigs.rb? Curious why it's not being triggered in this PR.
There was a problem hiding this comment.
Yes. It's not used yet because I didn't finish any of the nicer line-matching formatting in the parts the RBI gem is responsible for printing.
Instead, I just pad with as many blank lines as we need.
In a future iteration, this will be used to signal to the RBI gem that e.g. it should break a multi-line RBS sig into a multiline Sorbet sig.
There was a problem hiding this comment.
When we want to expose this from the CLI, yes we need to do that work. For now, we're only using it via the API, so it's enough for this PR.
| #| insert_pos: Integer, | ||
| #| sorbet_replacement: String? | ||
| #| ) -> void | ||
| def apply_class_annotation(annotation, parent_node:, insert_pos:, sorbet_replacement:) = raise |
There was a problem hiding this comment.
These are making this class harder to read but I'm not sure what we can do. One alternative I kinda like it to move these abstract methods to HumanReadableTranslator instead. Call sites in this class would then delegate to the appropriate "mode" that's being set, @translation_mode.apply_class_anotation. It wouldn't get rid of the abstract but make the intent more clear. Wdyt? Maybe as a future improvement to not hold up the PR.
There was a problem hiding this comment.
@amomchilov I'll leave this one for your assessment. I agree that something along the lines of what Kaan suggested would make the code a bit easier to follow.
There was a problem hiding this comment.
Same. Will follow-up.
amomchilov
force-pushed
the
Alex/line-matching
branch
2 times, most recently
from
a756398 to
886194e
Compare
vinistock
force-pushed
the
Alex/Options-object
branch
3 times, most recently
from
9d80524 to
48c4e23
Compare
vinistock
force-pushed
the
Alex/line-matching
branch
from
886194e to
e92832d
Compare
vinistock
force-pushed
the
Alex/line-matching
branch
from
e92832d to
7b7ef52
Compare
| def apply_member_annotations(annotations, sig) | ||
| known = [] #: Array[Spoom::RBS::Annotation] | ||
|
|
||
| annotations.each do |annotation| |
There was a problem hiding this comment.
I'll follow-up to tidy this with Array#map
KaanOzkan
left a comment
KaanOzkan
left a comment
There was a problem hiding this comment.
There are still 2 edge cases (one is hidden in a resolved comment) but I think the implementation is good. If it's not too hard it'd be good to try to fix these too.
| #: (Spoom::RBS::Signature, type_params: Array[::RBS::AST::TypeParam]) -> void | ||
| def rewrite_type_params_signature(signature, type_params:) | ||
| # Rewrite `#: [A, B]` into `# RBS_WRITTEN_ANNOTATION: [A, B]` | ||
| @rewriter << Source::Replace.new( |
There was a problem hiding this comment.
Similar to previous comment, multiline + annotation edge case is failing:
#: [A,
#| B]is translated into
# RBS_WRITTEN_ANNOTATION: [A,
#| B]
alexcrocha
left a comment
alexcrocha
left a comment
There was a problem hiding this comment.
Approving to unblock. We can fix forward any edge cases that come up
vinistock
force-pushed
the
Alex/line-matching
branch
from
55d9f6a to
e7964c9
Compare
4 participants
This PR adds a line preserving rewriter from RBS to Sorbet sigs, which is helpful when validating the runtime so that backtraces don't get shifted by the translation.
The idea is to introduce a new format, which changes the way we translate to not always simply delete the RBS, but instead keep artifacts that will maintain the original line numbers.