-
-
Notifications
You must be signed in to change notification settings - Fork 267
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
Support local
as an environment for Rails/UnknownEnv
from Rails 7.1 onward.
#894
Conversation
bb4562d
to
3db5d93
Compare
3db5d93
to
f6782da
Compare
RuboCop::Config.new( | ||
{ | ||
'AllCops' => { | ||
'TargetRailsVersion' => '7.1' |
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 missed this PR and added support for local
as part #906 as well. I think this approach is better, although it may make sense to cherry pick my commit introducing a :rails71
shared context (or I can open a separate PR for that, but it seems small) rather than a custom config, especially given the rest of this config is just the defaults.
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've extracted the shared context into its own PR in #1134.
@@ -0,0 +1 @@ | |||
* [#893](https://github.com/rubocop/rubocop-rails/issues/893): Support `local` as an environment for `Rails/UnknownEnv` from Rails 7.1 onward. ([@ghiculescu][]) |
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.
Does this warrant updating the cop's VersionChanged
config?
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.
Probably? I'm not very familiar with how this works. Do I add the next version number or is there some other workflow for this?
f6782da
to
9c9020c
Compare
…nEnv` from Rails 7.1 onward. Fixes rubocop#893
9c9020c
to
1eb9593
Compare
Rails 7.1 is a development version yet. So, the new feature for Rails 7.1 merge is pending until at least a beta, preferably a release candidate is shipped. |
Got it. Thanks. |
Rails 7.1.0.beta1 is out, and this feature (adding |
The release of RC is probably not far away. In principle, API will be stable once it becomes RC, so let's wait for a while. |
Makes sense! Also: |
context 'Rails 7.1' do | ||
let(:config) do | ||
RuboCop::Config.new( | ||
{ | ||
'AllCops' => { | ||
'TargetRailsVersion' => '7.1' | ||
}, | ||
'Rails/UnknownEnv' => { | ||
'Environments' => %w[ | ||
development | ||
production | ||
test | ||
] | ||
} | ||
} | ||
) | ||
end | ||
|
||
it 'accepts local as an environment name on Rails 7.1' 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.
Now that #1134 has been merged, I think this could be changed to use the shared context and cop_config
defined above. We also don't need to repeat "Rails 7.1" in the spec name, as it is already in the context name.
context 'Rails 7.1' do | |
let(:config) do | |
RuboCop::Config.new( | |
{ | |
'AllCops' => { | |
'TargetRailsVersion' => '7.1' | |
}, | |
'Rails/UnknownEnv' => { | |
'Environments' => %w[ | |
development | |
production | |
test | |
] | |
} | |
} | |
) | |
end | |
it 'accepts local as an environment name on Rails 7.1' do | |
context 'in Rails 7.1 onwards', :rails71 do | |
it 'accepts local as an environment name' 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.
@ghiculescu ping.
Thanks for the PR. I'm not going to be able to work on this for a little while, @sambostock or @koic are you interested in taking it over? Happy to close this PR and move to a new one if you like. |
Well, adjustments to the test code can be done separately. I'm going to merge this PR. Thank you @ghiculescu! |
Fixes #893
Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).bundle exec rake default
. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.md
if the new code introduces user-observable changes. See changelog entry format for details.