Handle on_sp when using prism
#241
Conversation
Earlopain
force-pushed
the
prism-on-sp
branch
from
16995b1 to
04357b2
Compare
| last&.type == :on_tstring_end | ||
| return false unless last | ||
|
|
||
| last.type == :on_tstring_end || (last.type == :on_sp && last.token == TRAILING_SLASH) |
There was a problem hiding this comment.
The (last.type == :on_sp && last.token == TRAILING_SLASH) part is to handle the fact there is no Prism 1.8.1/1.9.0 yet and so no easy way to detect Prism has :on_sp vs not, right?
There was a problem hiding this comment.
Yeah, that's right. I tried feature detection (Prism.lex_compat(" ").value.dig(0, 1) == :on_sp) but then some other test here fails because prism loads delegate and that adds a method to Kernel which it didn't like
There was a problem hiding this comment.
Yeah, that's right. I tried feature detection (Prism.lex_compat(" ").value.dig(0, 1) == :on_sp) but then some other test here fails because prism loads delegate and that adds a method to Kernel which it didn't like
Explaining that test a bit: I have regression tests that make sure I'm not accidentally messing with people's requires so they don't come to rely on me loading something by accident. Otherwise, if I require pathname someone might come to rely on that always being available and have their code break if I were to change usage.
The two ways to use syntax suggest is either by requiring syntax_suggest which sets up core extensions that only trigger those loads on syntax error. Or you can also require syntax_suggest/api which was added for support for sorbet (not sure if they ended up using it or not) so they can manually invoke syntax suggest on parsing.
The test that fails is when requiring syntax_suggest/api, which is less worrisome than the more common syntax_suggest (loaded by default). But it's still better not to do it at all.
|
Some CI jobs are failing, the 4.0 job failures seem unrelated & pre-existing but https://github.com/ruby/syntax_suggest/actions/runs/21170190500/job/60887146934?pr=241 needs a changelog entry or so |
Earlopain
force-pushed
the
prism-on-sp
branch
from
04357b2 to
d34cef4
Compare
|
CI should be green with #242. I added a changelog entry |
It used to not emit this token type, but now it does. So when a newer version of prism is present, we can fall back to the same code that ripper uses. Ref: * ruby/ruby#15914 * ruby/prism#3859
Earlopain
force-pushed
the
prism-on-sp
branch
from
d34cef4 to
94d27c4
Compare
|
What's the order here, I suppose we could merge this PR, and that will then auto-push/sync a commit to ruby/ruby and then we only need the Reapply commit from ruby/ruby#15914? Also not sure how urgent it is, I suppose it can get messy if other PRs are merged in ruby/prism as then that sync might not work either. If it's not urgent I would probably wait for @schneems' review. |
|
I think we can merge the ruby PR, leave this open, and then apply any desired changes on top of this PR so that those get synced over. We should not merge much in prism related to ripper until the commit is reapplied. Since we know, we can wait a bit until we get a review here. |
|
Makes sense, I merged the ruby/ruby PR. |
The reason this logic for different methods branches in the class instead of internally was to be eagerly aggressive about runtime performance. This code is currently only used once for the document where it's invoked ~N times (where N is number of lines):
```ruby
module SyntaxSuggest
class CleanDocument
# ...
def join_trailing_slash!
trailing_groups = @document.select(&:trailing_slash?).map do |code_line|
take_while_including(code_line.index..) { |x| x.trailing_slash? }
end
join_groups(trailing_groups)
self
end
```
Since this is not currently a hot-spot I think merging the branches and using a case statement is a reasonable tradeoff and avoids the need to do specific version testing.
An alternative idea was presented in #241 of behavior-based testing for branch logic (which I would prefer), however, calling the code triggered requiring a `DelegateClass` when the `syntax_suggest/api` is being required.
|
I added tests for multiple prism versions in #243 then cherry-picked your commit on top, and re-worked the implementation a bit after reviewing why I split my branching logic like that. Can we flip the script and have you all review my changes in #243 to make sure things are clear (and suggestions welcome). |
3 participants
It used to not emit this token type, but now it does. So when a newer version of prism is present, we can fall back to the same code that ripper uses.
Ref:
on_spinsyntax_suggestruby#15914