-
Notifications
You must be signed in to change notification settings - Fork 16
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
Detect references to ENV outside of config directories #71
base: main
Are you sure you want to change the base?
Conversation
woaow this is a great idea! gonna take a peek 🙂 |
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.
domain lgtm, it all looks straightforward to me. only thing I would consider is a more precise name over Betterment/Environment, but I don't feel strongly enough about it to block.
what's your plan to roll this out? we use ENV in a lot of places, in models and controllers and their specs, etc
The plan is to:
|
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.
platform LGTM - pending naming choice raised by @6f6d6172.
README.md
Outdated
@@ -130,6 +130,27 @@ end | |||
``` | |||
|
|||
All three `params.permit` calls will be flagged. | |||
|
|||
### Betterment/Environment |
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.
### Betterment/Environment | |
### Betterment/EnvironmentConfiguration |
If you're looking for a slightly more descriptive name.
Invalidated by push of 05f9eee
This cop will report references to
ENV
in files outside of the config directory.The idea is that applications should extract variables from the ENV at boot time, and fail if those variables are not valid.