-
Notifications
You must be signed in to change notification settings - Fork 11
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
Centralize Ruby Version to .ruby-version
#115
Conversation
fb827c0
to
9dfc993
Compare
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 looks fine to me, afaict no substantive changes really and green CI 👍
test/app_profiler/profile_test.rb
Outdated
@@ -153,7 +153,7 @@ class ProfileTest < TestCase | |||
end | |||
|
|||
test "#file uses default prefix format when no custom profile_file_prefix block is provided" do | |||
travel_to Time.zone.local(2022, 10, 06, 12, 11, 10) do | |||
travel_to Time.zone.local(2022, 10, 0o6, 12, 11, 10) do |
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.
Odd choice for the cop - surely 6
is a way more readable fix here 🤷
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 are quite a lot of changes not related to adding .ruby-version in here, which in particular will conflict in several places with #101 that i've been trying for months to get merged.
Can we reduce the scope of this PR to just add the .ruby-version file, and do these other fixes in another PR, ideally after #101 merges?
@dalehamel that makes sense! Here's the PR #119. Let me know when #101 merges and I can apply the remaining changes. |
@jenshenny sorry for the conflicts here. I resolved the ones on #119 and added 3.3 to the CI matrix and merged it. Could you please rebase and do another pass? Thanks for waiting to #101 to land. |
will be read from required_ruby_version after this version
Layout/FirstArgumentIndentation Layout/EmptyLinesAroundExceptionHandlingKeywords Layout/MultilineArrayLineBreaks Layout/FirstHashElementLineBreak Layout/FirstMethodArgumentLineBreak Layout/MultilineHashKeyLineBreaks
Style/TrailingCommaInArguments Style/NumericLiteralPrefix Style/RedundantReturn Style/ClassMethodsDefinitions
9dfc993
to
c4e07b2
Compare
@dalehamel rebased! Sorry for the delay, was on vacation for the last little while and wasn't able to get to it until now |
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.
Thanks!
What are you trying to accomplish?
The
.ruby-version
file is the ecosystem standard for defining a Ruby version. This PR adds the.ruby-version
file, ensures arequired_ruby_version
is set, and removes all other references to Ruby in this repository, aligning it with the standard.What should reviewers focus on?
Important
Please verify the following before merging:
Verify that the changes in the PR meets the following requirements or adjust manually to make it compliant:
.ruby-version
file is present with the correct Ruby version definedrequired_ruby_version
in your gemspec is setdev.yml
Ruby task (before:- ruby: x.x.x
, after:- ruby
)Gemfile
(no lines withruby <some-version>
)Gemfile.lock
is built with the defined Ruby versionTargetRubyVersion
defined inrubocop.yml
(reads fromrequired_ruby_version
on Rubocop 1.61.0)ruby/setup-ruby
Github Actions that do not run on a Ruby matrix (no lines withruby-version: “x.x”
)To establish consistency, the
required_ruby_version
is set to 2.7. If you think that another version is a better fit, please make the applicable changes.Please merge this PR if it looks good, this PR will be merged if there isn't any activity after 4 weeks.