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 Crystal::Environment module #5261

Closed
wants to merge 2 commits into from

Conversation

Sija
Copy link
Contributor

@Sija Sija commented Nov 8, 2017

This PR supplements stdlib with basic functionality around ENV["CRYSTAL_ENV"] variable.

# handy alias for Crystal::Environment
Crystal.env # => "development"

# getter with "development" as default value
Crystal.env.name # => "development"

# setter
Crystal.env.name = "test"

# query methods
Crystal.env.development? # => false
Crystal.env.test?        # => true
Crystal.env.staging?     # => false
Crystal.env.production?  # => false

Closes #5220.


Code from this PR ATM lives in a shard, available at https://github.com/Sija/crystal-environment.

@Sija Sija force-pushed the feature/crystal-environment branch from 69baec5 to 22200f7 Compare November 8, 2017 15:35
# ```
module Environment
ENV_KEY = "CRYSTAL_ENV"
ENV_VALUES = %i(development test staging production)
Copy link
Contributor

Choose a reason for hiding this comment

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

What if I want other environment names? What if I don't want staging, because I consider that staging == production?

Copy link
Contributor Author

@Sija Sija Nov 8, 2017

Choose a reason for hiding this comment

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

These values are used only to auto-generate query methods like Crystal.env.development?.

In 99% of cases these should be enough, but if you don't need/want to use those, you can always do Crystal.env.name == "foo" to check for non-standard env.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not nice:

if Crystal.env.staging? || Crystal.env.name == "integration"
  # do some stuff
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, in terms of solutions we could add Crystal::Environment.setup(keys) macro replacing/extending defaults, or use method_missing (which was deemed a bad idea before).

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea for a setup macro. Though I think the default envs are going to be enough in most cases

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I think staging should be removed. Amber uses dev/test/prod and so does Elixir’s mix, Rails etc. Heroku even recommends against staging. So I think the main 3 should be there and people can add others if they want. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kostya
Copy link
Contributor

kostya commented Nov 8, 2017

why this needed? this is app related stuff, not crystal itself. better to create nice app generator template like crystal init app ... with such things included.

@Sija
Copy link
Contributor Author

Sija commented Nov 8, 2017

@kostya see discussion in #5220.

@Sija Sija mentioned this pull request Nov 10, 2017
@akzhan
Copy link
Contributor

akzhan commented Nov 15, 2017

bad, confusing naming.

I suppose it should be part of config shard.

@RX14
Copy link
Contributor

RX14 commented Nov 18, 2017

Closed because #5220 was closed.

@RX14 RX14 closed this Nov 18, 2017
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.

None yet

7 participants