I saw case of assigning to environment variable required by library (e.g.: ENV['REQUIRED_BY_LIBRARY'] = app_specific_value). In other words, this cop will prohibit only reference to ENV. How about allowing assignments to ENV?

I came up with a mundane idea of splitting the cop by reference and assignment to ENV, or providing options :-)

I saw case of assigning to environment variable required by library (e.g.: ENV['REQUIRED_BY_LIBRARY'] = app_specific_value). In other words, this cop will prohibit only reference to ENV. How about allowing assignments to ENV?

I can add a configuration option. In the interest of time, I think I will limit to #= and assume ENV is always flat (not nested.) 🙂

I think I will limit to #= and assume ENV is always flat (not nested.)

I think it's good enough :-)

@koic Added!

Not sure why JRuby is failing. Seems unrelated to the change.

@Drenmi Can you rebase with the latest master branch and update this PR?

Will have a look this week. 🙏

@Drenmi I plan to release new RuboCop Rails in a few days. Is it possible to make progress this PR?

Sorry for the delay, @koic. I've been caught up with work. I just rebased this and incorporated the suggested documentation.

@Drenmi Thank you for updating! 😃

@Drenmi how do you feel about ENV.fetch() with a fallback? ex: ENV.fetch('UNDEFINED_ENV_VAR', 'will return this'). I just upgraded and this cop is currently flagging these calls, but I don't see them as problematic because they don't cause runtime errors.

Hey, @swrobel. I initially had this response, but I've come around to the idea and am changing my code to comply with the cop. You're right that fetch with fallback wouldn't cause runtime errors, however the secondary benefit that would be missed (if fetch with fallback was permitted) is (from above);

We had no place where we could see all the required ENV variables at one glance.

I find this really handy; it informs which variables my environment needs, without needing to rummage through the whole codebase to check if there are any ENV.fetch, etc. (that I wouldn't otherwise notice until runtime) and prevents me accidentally forgetting to add a new (optional or required) env var to a .env.template file which is commonly used with dotenv.

You could of course use this to have the same effect if helpful;

# in initializer or config setup
Rails.application.config.my_undefined_env_var = nil
# deep in code somewhere
Rails.application.config.my_undefined_env_var || 'will return this'

What do you think?

While I'm not against this cop itself, it was an unpleasant surprise to stumble upon after a rubocop-rails update.

I need to invent my own thing to make it pass, and while that's surely possible - based on the PR-description and comments - I don't think it's ideal.

There need to be a corresponding best practice example for this cop, in the Rails Style Guide, or somewhere else. As it is now I fear this cop will mostly lead to fragmentation and wasted time.

@Drenmi can you provide example code for how you're loading your env vars via environment.yml? While this cop does provide reasoning behind its purpose, it doesn't give developers a migration path.

@Drenmi Could you write a migration guide to a good case? Probably due to user confusion, I'm considering disabling the cop by default if migration explanation doc is not yet in place by the next release.