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 new Rails/Env cop #1375

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add new Rails/Env cop #1375

wants to merge 1 commit into from

Conversation

cdudas17
Copy link

@cdudas17 cdudas17 commented Oct 10, 2024

Add new Rails/Env cop to discourage code paths from checking Rails.env.environment?


Closes #1376

This Rubocop rule is relevant to the following Rails Style Guide rules as it encourages people to use the solutions instead of checking Rails.env.environment?
Dev/Test/Prod Configs
Staging Like Prod
YAML Config

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.

@cdudas17 cdudas17 force-pushed the master branch 2 times, most recently from 3a45198 to 65bf185 Compare October 10, 2024 22:23
@cdudas17 cdudas17 changed the title Add new Rails/Env cop Add new Rails/Env cop. Oct 10, 2024
@cdudas17 cdudas17 changed the title Add new Rails/Env cop. Add new Rails/Env cop Oct 10, 2024
@cdudas17 cdudas17 marked this pull request as ready for review October 10, 2024 22:54
Copy link
Contributor

@Earlopain Earlopain left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I agree with the general idea of what the cop is about but question if it should be enabled by default.

Checking the env directly seems like a somewhat common thing to do, and resolving the offense is not trivial. I can also see how some would just not care about feature-flags or similar at all, especially smaller applications. You did write in your issue that this is explicitly about large apps. In my opinion the cop should be opt-in instead.


Rails.env.capitalize is a offense, can be used to display in a dashboard of sorts. I think it probably fine to only display warnings for predicate methods.

lib/rubocop/cop/rails/env.rb Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
changelog/new_env_cop.md Outdated Show resolved Hide resolved
@cdudas17
Copy link
Author

@Earlopain That makes sense that the cop should be opt-in. I changed the config default from pending to false. I also updated some other comments you had. Rails.env.capitalize is no longer an offense, and I added a test for it. We are looking for predicate methods now and including those methods as the offending node.

@cdudas17
Copy link
Author

@Earlopain after looking into more predicate methods on Rails.env we found there were probably more valid uses even within predicated methods. To make the rule still be able to check for custom environments that have been added like Rails.env.demo_environment?, we chose to add an allow list of predicate methods to the cop instead of a block list.

@Earlopain
Copy link
Contributor

Earlopain commented Oct 15, 2024

Hm, yeah. That's a good point there. I don't particularly like the solution, Rails/UnknownEnv already knows about the available envs (courtesy of the user configuring it) and it feels like that should have an affect here.

But I don't believe there is precedence for overarching configuration like that. So, I'd just leave it like you have it now. Though I think your list includes a cusom core extension? contains_mb4_chars doesn't sound familiar and I find no reference to it anywhere. colorized? maybe from a gem called colorized? Here are my results:

require "active_support/all"
("".inquiry.methods - Object.instance_methods).select { |m| m.to_s.end_with?('?') }
=> [:unicode_normalized?, :exclude?, :empty?, :acts_like_string?, :include?, :is_utf8?, :casecmp?, :match?, :starts_with?, :ends_with?, :start_with?, :end_with?, :valid_encoding?, :ascii_only?, :between?]

Methods like that seem fine to not appear, as long as the common ones like start_with?/end_with? are there. Notice how I use instance_methods instead, which makes include? appear as a potential canidate. It previously got filtered out because Object through Module responds to it.

@cdudas17
Copy link
Author

@Earlopain The allow list isn't my favorite solution either but I'm not sure what a better solution would be. I'll defer to your decision about overreaching in configuration. I updated the allow list to be yours and confirmed I had the same list of predicate methods in a more vanilla rails app.

@Earlopain
Copy link
Contributor

This looks good to me, I have no more comments. I'm not a maintainer, so in the end the harder decisions are (luckily) not up to me (:

@cdudas17
Copy link
Author

@Earlopain is there workflow I'm missing to get this properly approved? I read https://github.com/rubocop/rubocop-rails/blob/master/CONTRIBUTING.md and think I'm following all the steps. Do I need to request from someone specifically or am I missing a step that you're aware of? You're my only point of contact in this repo so far 😅

@Earlopain
Copy link
Contributor

Sorry for the late response, I forgot. You didn't miss anything, it's just that adding new cops here can be a bit of a slog. If you look at the currently open PRs, most are for new cops. Can't give you anything other than being patient I'm afraid.

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.

Cop idea: Rails/Env
2 participants