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
Update ripper_supported? for truffleruby and ripper specs #395
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This is not ideal, because TruffleRuby might support
Ripper
at some point.How about checking
Ripper.respond_to?(:lex)
which is what https://github.com/k0kubun/hamlit does for instance?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.
You could push that onto the stack for if run truffle ruby?
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.
@eregon I think there would be an issue with this because part of the spec asserts
Ripper
would not be loaded by feature detection:rspec-support/spec/rspec/support/ruby_features_spec.rb
Lines 151 to 153 in b83b4ce
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 see, that is unfortunate. Probably fine to keep as-is then and change it again (e.g., to a version check) once TruffleRuby supports Ripper.
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.
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.
Right, so the whole point of this logic is then avoiding
require 'ripper'
if Ripper is unsupported.Is that to avoid changing
$LOADED_FEATURES
or making sure code which uses Ripper stillrequire 'ripper'
, but only for Rubies with unsupported Ripper?I would suggest something a lot more straightforward by always requiring Ripper, then we wouldn't need to have so much Ruby-implementation-specific code here.
I don't see that:
I wouldn't want that, I'd want the same behavior as MRI.
Since anyway this does not seem a critical feature of RSpec, I propose to adapt the code here once TruffleRuby supports Ripper and merge the PR as it is.
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's to avoid requiring ripper if possible.
We do the least requiring we can to avoid false positives due to poisoned environments.
I stand corrected,
irb
requires it,ruby
does not.In my experience only MRI behaves like MRI, other Ruby engines always have quirks. 😢
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.
FWIW, TruffleRuby tries hard to behave exactly like MRI. But implementing Ripper is a significant effort, so that's not done yet.
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.
To be fair I should probably say that every engine has quirks, it's just the MRI ones are the reference quirks? 🤷♂
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 just wanted to end on a positive note :)
This PR is not the most beautiful code but TruffleRuby otherwise runs RSpec (and Rails nowadays) unmodified with no TruffleRuby-specific code since many years now.
We try hard to avoid any TruffleRuby-specific code in gems.