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

Lucky env predicates #13

Merged
merged 3 commits into from
Jul 5, 2021

Conversation

hovsater
Copy link
Contributor

@hovsater hovsater commented Jul 4, 2021

This PR addresses parts of #4 by moving the Lucky::Env code into lucky_env itself.

src/lucky_env.cr Outdated
@@ -23,4 +23,18 @@ module LuckyEnv
load(file_path)
end
end

def self.name
Copy link
Contributor Author

@hovsater hovsater Jul 4, 2021

Choose a reason for hiding this comment

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

I don't particularly like calling it name when it's on the shard namespace, but couldn't really think of anything better. I think name was fine, when it was nested under Lucky::Env though.

If anyone have a suggestion, I'd love to hear it. 🙂

Copy link
Member

Choose a reason for hiding this comment

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

How about environment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I addressed this in 6ad252b. 🙂

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

This is great! I agree with the name method comment. It was fine when in the Lucky app under Lucky::Env, but being in the shard might be a little more confusing. Maybe something like .environment might be more clear?

@paulcsmith what are your thoughts on this?

src/lucky_env.cr Outdated
@@ -23,4 +23,18 @@ module LuckyEnv
load(file_path)
end
end

def self.name
Copy link
Member

Choose a reason for hiding this comment

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

How about environment?

src/lucky_env.cr Outdated
ENV.fetch("LUCKY_ENV", "development")
end

{% for env in %w[development test production] %}
Copy link
Member

Choose a reason for hiding this comment

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

Can you add staging to this? I think that will cover the 99% use case, then later on we can figure out a nice way to make it more configurable at compile-time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jwoertink I can, but we don't seem to include it by default right now, so I chose not to. However, I just added 6ad252b which makes it easier to extend LuckyEnv with custom environments. So instead of hardcoding a list within LuckyEnv itself, we can now just call LuckyEnv.add_env :staging to add this environment as well.

Comment on lines +144 to +149
private def with_env(key, value, &block)
original_value = ENV[key]?
ENV[key] = value
block.call
ensure
ENV[key] = original_value
Copy link
Member

Choose a reason for hiding this comment

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

Nice! Love this.

This will make it easier to extend LuckyEnv with custom environments.
Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

👍

Comment on lines +8 to +13
macro add_env(name)
def LuckyEnv.{{ name.id }}?
environment == {{ name.id.stringify }}
end
end

Copy link
Member

Choose a reason for hiding this comment

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

Oh nice! Ok, this is awesome. This means in a generated app, we can have the config/env.cr file be some comments on how to use this to configure additional envs plus document using these methods 🥳 Good call ❤️

@jwoertink
Copy link
Member

Thanks for adding this in! I've opened up luckyframework/lucky_cli#654 to address what the generated apps should do now.

@jwoertink jwoertink mentioned this pull request Jul 6, 2021
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.

2 participants