Skip to content
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

Add Rails/EnvLocal cop to enforce upcoming Rails.env.local? #906

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

sambostock
Copy link
Contributor

@sambostock sambostock commented Jan 6, 2023

rails/rails#46786 introduces Rails.env.local? as a helper for Rails.env.development? || Rails.env.test?, which should be released as part of the next Rails release (presumably 7.1, although possible 8).

This PR includes the following to support this:


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.
  • If this is a new cop, consider making a corresponding update to the Rails Style Guide.
    Good idea!

@sambostock sambostock force-pushed the rails-env-local branch 4 times, most recently from 1004ce3 to 6b4cbc2 Compare January 6, 2023 05:44
@sambostock
Copy link
Contributor Author

sambostock commented Jan 6, 2023

Oh, I see #894 uses an alternative approach to add support for .local? to Rails/UnknownEnv, albeit differently. Both approaches have tradeoffs:

PR Approach Tradeoff
#906 (this PR) Add local to config Blindly allows local in Rails versions that don't support it
#894 Injects local into config if Rails version >= 7.1 Impossible to disallow local, if desired

As I type this, I realize maybe the tradeoff in #894 is actually just best addressed by me making this cop have a configurable enforced style (local? vs development? || test?)... 🤔

@sambostock sambostock changed the title Handle upcoming Rails.env.local? Add Rails/EnvLocal cop to enforce upcoming Rails.env.local? Jan 6, 2023
@sambostock
Copy link
Contributor Author

sambostock commented Jan 6, 2023

I've updated the PR as follows:

# Rails.env.local?
#
# # good
# Rails.env.development? || Rails.env.test?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The EnforcedStyle: no_local option looks unnecessary. So, it would be sufficient to provide only the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My justification was that authors may prefer to be explicit (or perhaps they need to support an old Rails version, in the case of a gem author) and not want to use local?. Normally this would be covered by updating the UnknownEnv config, except that #894's implementation always injects it in if the target Rails version is >= 7.1.

However, if you think that's too obscure, I can certainly remove the EnforcedStyle option.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not generally downgraded supported version of Rails. If user don't want to replace with this Rails.env.local?, just can disable this cop. So I think it would be better to remove this EnforcedStyle: no_local and keep it simple.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍

@koic
Copy link
Member

koic commented Jan 26, 2023

This looks good. However, Rails 7.1 is a development version yet. So, the new cop merge is pending until at least a beta, preferably a release candidate is shipped.

@koic
Copy link
Member

koic commented Jan 26, 2023

Add :rails71 shared context so specs can be written against Rails 7.1

Please open a PR only for c856fa1 if you like. Rails 7.1 development has started, so it can be merged independently.

@sambostock sambostock mentioned this pull request Sep 28, 2023
5 tasks
@sambostock
Copy link
Contributor Author

@koic I've opened #1134 now Rails 7.1 release candidate 1 has been released.

@koic
Copy link
Member

koic commented Oct 2, 2023

@sambostock #1134 was merged. Can you rebase with the latest master branch?

Enforces the use of `Rails.env.local?` instead of
`Rails.env.development? || Rails.env.test?`, as of Rails 7.1.

def on_or(node)
rails_env_local_candidate?(node) do |*environments|
next unless environments.to_set == LOCAL_ENVIRONMENTS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! You're too fast for me! 😅

@sambostock sambostock requested a review from koic October 2, 2023 02:56
@koic koic merged commit b1ff70b into rubocop:master Oct 2, 2023
4 checks passed
@koic
Copy link
Member

koic commented Oct 2, 2023

Thanks!

@sambostock sambostock deleted the rails-env-local branch October 2, 2023 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants