-
-
Notifications
You must be signed in to change notification settings - Fork 423
Add support for Ruby v4.0 #1643
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
base: main
Are you sure you want to change the base?
Changes from all commits
d139fba
aabd91e
9b99dee
3265ff3
9172bd4
db91a98
7af1aaf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,4 +20,8 @@ Gem::Specification.new do |s| | |
| s.executables = ['yard', 'yardoc', 'yri'] | ||
| s.license = 'MIT' if s.respond_to?(:license=) | ||
| s.metadata['yard.run'] = 'yri' | ||
| if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('4.0.0') | ||
| s.add_dependency 'irb' | ||
| s.add_dependency 'logger' | ||
| end | ||
|
Comment on lines
+23
to
+26
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The conditional here doesn't really do the "right" thing, namely for rubygems this will require Ruby 4.x to build the deps, which means gem publishes need to happen from Ruby 4 otherwise deps will get lost. That seems like it would be very error prone and lead to regressions. That said, I understand what we're trying to do and I think the biggest blocker for this change is that the only way to do this is to force the added deps to all Rubies, and that might be a bit of a problem.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe add another gemspec for a different gem name, e.g. yard_on_ruby4, and set required_ruby_version to 4.0.0...?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or vendor the code from the dependencies?
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting, but I can't imagine how that would work from a user POV. At that point, directing users to install IMO I think the more likely approach might actually be to vendor a lot of these libraries to avoid any extra complexity. Unfortunately the Ruby maintainers didn't provide a lot of backward compatibility options when moving down this path of these breaking changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That or adding the decencies for everyone. That will just force Ruby 3.x users to use other version of the concerned libs. Probably using a more up to date version than the one natively embedded in their ruby 3.x system. |
||
| end | ||
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.
Curious why ostruct here and not in the gemspec?
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.
As far as I could see ostruct is only used in one of the benchmarking scripts and I don't think they're distributed in the gem, are they?
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.
Oh indeed we did move away from OpenStruct in the ~ last release.