Adds environment boolean checks #302

eliasjpr · 2017-10-12T01:19:47Z

Amber currently ships with 3 different environments: development, test
and production.

As a developer I would like to sometimes run code conditionally
depending on the environment that the application is.

if Amber.env.development?
  # ... code here ...
end

This PR addresses the issue by creating a Amber::Env module that
contains 3 module methods:

Amber.env.development?
Amber.env.test?
Amber.env.production?
Amber.env.integration?
Amber.env.staging?
Amber.env.sandbox?

Amber.env.in? %w(development test)
Amber.env.in? %i(development test)

Amber.env.is? "test"
Amber.env.is? :test

With the changes in this PR a developer should be able to call the
appropriate method to check for the desired environment.

elorest · 2017-10-12T01:51:01Z

Could we add it directly to Amber like it is in Rails. So Amber.env Amber.development? etc.

eliasjpr · 2017-10-12T02:03:08Z

I considered Amber.development? and do like the aesthetics of this, but then I thought that if we move the check all the way, would people expect all the settings to live in this scope? So I opted to be safe by introducing the Amber::Env I consider the module approach to be more appropriate Amber::Env since it allow us to extend the module we can more methods and stuff pertinent to the Env here if we wish to in the future.

elorest · 2017-10-12T03:51:19Z

I agree we shouldn't mimic or copy rails in a lot of things, but in this case I do like the way rails does it. Rails.env automatically calls to_s. This is the same for every instance in crystal and ruby.
https://carc.in/#/r/2w6a

You might be right though. If you prefer it this way I'll approve it.

veelenga · 2017-10-12T12:40:08Z

@eliasjpr looks nice 👍

What about user-defined environments? staging, integration, sandbox, beta etc. often can be seen in web apps. I understand it is not possible to dynamically generate .staging? method, but it could be good to have some kind of fallback for this, for example, Amber::Env.is? "staging" or Amber::Env["staging"]? or something more friendly...

migeorge · 2017-10-12T13:08:06Z

@eliasjpr The implementation looks good 👍. FYI there is a test failing though.

drujensen · 2017-10-12T13:15:03Z

@veelenga really like the Amber::Env.is? :staging idea.

veelenga · 2017-10-12T13:39:04Z

Another use case might be to check for the inclusion in a list of environments.

Amber::Env.in? %w(development test) or
Amber::Env.in? %i(development test)

which is more or less consistent with Amber::Env.is?. WDYT?

eliasjpr · 2017-10-12T19:06:07Z

I have added the in? and is? methods as suggested @veelenga Thanks, this is a great addition

It would be nice to populate the ENVIRONMENTS constant at precompile time appending the contents of the ./config/environments files present. This way the ENVIRONMENTS will contain the all the environments defined.

I attempted to approach the problem with method_missing and converting the Env to a class, so the call would look something like Amber::Env.new.development?. method_missing will not work in modules, the macro is invoked when a method call on an instance fails, modules cannot be instantiated. What do you guys think about Amber::Env.new.development? it works nice for all environments and there is no need for the ENVIRONMENTS constant, but the aesthetics of the call is too verbose in my opinion.

I'm going to look at the failing specs.

elorest

My only concerns here are that Amber::Env.is?("development") doesn't seem like much of an improvement over ENV["AMBER_ENV"] == "development" which already works. It might seem cleaner this way, but could we just create methods which return the existing VAR instead of storing the value multiple times?

elorest · 2017-10-12T19:31:34Z

src/amber/server/env.cr

@@ -0,0 +1,27 @@
+module Amber
+  module Env
+    ENVIRONMENTS = %w(development test production)


These could be dynamically defined at compile time by looking at file names in ./config/environments

Do you have an example on how to do this? I got stuck...

Yeah. Give me a second.

There are 2 ways to do this.

Eval

This way requires less files but isn't quite as fast for compile time.

ENVIRONMENTS = {{ system("crystal eval \'puts Dir.glob(\"./config/environments/**\").map(&.split(/[\\/\\.]/)[-2]?)\'") }}

New File using Macro Run

This way is a bit faster as the compile can cache the compiled environment_names.cr

# environment_names.cr puts Dir.glob("./config/environments/**").map(&.split(/[\/\.]/)[-2]?)

Then call ENVIRONMENTS = {{run("./path/to/environment_names.cr")}} in env.cr.

elorest · 2017-10-12T19:41:59Z

src/amber/server/env.cr

+    end
+
+    def self.current
+      Settings.env.not_nil!.downcase


Store env here is just duplicating values already stored in ENV["AMBER_ENV"]. Is this necessary? I would rather current just wrap ENV["AMBER_ENV"] and return it's values.

I would trust the Settings.env more than the ENV["AMBER_ENV"] since the latter can be modified outside of the APP after it has been started. What do you think?

ENV["AMBER_ENV"] is defined by the env var at runtime and can't be changed outside of the app afterwards. Since the env var is where this value originates I forsee some possible issues where they could get out of sync and cause confusion if they're not pointing the same value.

We should be fine using Settings.env since its pointing to ENV["AMBER_ENV"]. See https://github.com/amberframework/amber/pull/302/files#diff-b8648cef4f24d74b1b3b0cee94f1c9deR15

Ok. I'm ok with that then.

eliasjpr · 2017-10-13T00:20:58Z

Updates.

@elorest was right by using the Rails way it turned out to be very clean and feature rich

kudos @elorest 🎉

This now supports the following.

Amber.env.development?
Amber.env.test?
Amber.env.production?
Amber.env.integration?
Amber.env.staging?
Amber.env.sandbox?

Amber.env.in? %w(development test)
Amber.env.in? %i(development test)

Amber.env.is? "test"
Amber.env.is? :test

elorest · 2017-10-13T00:28:58Z

spec/amber/scripts/environment_loader_spec.cr

@@ -0,0 +1,47 @@
+require "../../../spec_helper"
+
+describe Amber::Server do


I like the idea of moving this under scripts. Would it be possible to move it and maintain git file history though instead of adding a new file?

I can undo the commit, and you can go ahead and move it in a separate PR. WDYT?

Amber currently ships with 3 different environments: development, test and production. As a developer I would like to sometimes run code conditionally depending on the environment that the application is. ```crystal if Amber.env.development? # ... code here ... end ``` This PR addresses the issue by creating a Amber::Env module that contains 3 module methods: ```crystal Amber.env.development? Amber.env.test? Amber.env.production? Amber.env.integration? Amber.env.staging? Amber.env.sandbox? Amber.env.in? %w(development test) Amber.env.in? %i(development test) Amber.env.is? "test" Amber.env.is? :test ``` With the changes in this PR a developer should be able to call the appropriate method to check for the desired environment. Adds Additional Module methods `in?` and `is?` methods to check for inclusion of the environments Uses rails style for Amber environment checks

eliasjpr requested a review from elorest October 12, 2017 01:19

eliasjpr force-pushed the ep/evironment-check-methods branch from 8271c32 to f515cf8 Compare October 12, 2017 01:29

eliasjpr requested review from drujensen, migeorge, fridgerator, c910335 and a team October 12, 2017 01:29

eliasjpr force-pushed the ep/evironment-check-methods branch from f515cf8 to 55413d4 Compare October 12, 2017 02:13

fridgerator approved these changes Oct 12, 2017

View reviewed changes

migeorge approved these changes Oct 12, 2017

View reviewed changes

eliasjpr force-pushed the ep/evironment-check-methods branch 4 times, most recently from b482070 to 10ba651 Compare October 12, 2017 19:38

elorest reviewed Oct 12, 2017

View reviewed changes

eliasjpr force-pushed the ep/evironment-check-methods branch from 10ba651 to 5d78679 Compare October 12, 2017 19:45

eliasjpr force-pushed the ep/evironment-check-methods branch from dae7954 to 96e7b9b Compare October 13, 2017 00:25

elorest reviewed Oct 13, 2017

View reviewed changes

eliasjpr force-pushed the ep/evironment-check-methods branch 2 times, most recently from 2b2ca30 to 9d502a4 Compare October 13, 2017 00:40

eliasjpr force-pushed the ep/evironment-check-methods branch from 9d502a4 to 3058844 Compare October 13, 2017 00:51

eliasjpr merged commit 14f56cf into master Oct 13, 2017

eliasjpr deleted the ep/evironment-check-methods branch October 13, 2017 01:21

faustinoaq mentioned this pull request Nov 15, 2017

[WIP] Implement error generator (#294) #345

Closed

faustinoaq added this to Done in Framework 2018 May 5, 2018

faustinoaq removed this from Done in Framework 2018 Jun 12, 2018

faustinoaq added this to Done in Amber Framework 2017 Jun 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds environment boolean checks #302

Adds environment boolean checks #302

eliasjpr commented Oct 12, 2017 •

edited

elorest commented Oct 12, 2017

eliasjpr commented Oct 12, 2017 •

edited

elorest commented Oct 12, 2017 •

edited

veelenga commented Oct 12, 2017 •

edited

migeorge commented Oct 12, 2017

drujensen commented Oct 12, 2017

veelenga commented Oct 12, 2017 •

edited

eliasjpr commented Oct 12, 2017 •

edited

elorest left a comment •

edited

elorest Oct 12, 2017

eliasjpr Oct 12, 2017

elorest Oct 12, 2017

elorest Oct 12, 2017

elorest Oct 12, 2017

eliasjpr Oct 12, 2017

elorest Oct 12, 2017 •

edited

eliasjpr Oct 12, 2017

elorest Oct 12, 2017

eliasjpr commented Oct 13, 2017

elorest Oct 13, 2017

eliasjpr Oct 13, 2017

		@@ -0,0 +1,47 @@
		require "../../../spec_helper"

		describe Amber::Server do

Adds environment boolean checks #302

Adds environment boolean checks #302

Conversation

eliasjpr commented Oct 12, 2017 • edited

elorest commented Oct 12, 2017

eliasjpr commented Oct 12, 2017 • edited

elorest commented Oct 12, 2017 • edited

veelenga commented Oct 12, 2017 • edited

migeorge commented Oct 12, 2017

drujensen commented Oct 12, 2017

veelenga commented Oct 12, 2017 • edited

eliasjpr commented Oct 12, 2017 • edited

elorest left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eval

New File using Macro Run

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elorest Oct 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliasjpr commented Oct 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliasjpr commented Oct 12, 2017 •

edited

eliasjpr commented Oct 12, 2017 •

edited

elorest commented Oct 12, 2017 •

edited

veelenga commented Oct 12, 2017 •

edited

veelenga commented Oct 12, 2017 •

edited

eliasjpr commented Oct 12, 2017 •

edited

elorest left a comment •

edited

elorest Oct 12, 2017 •

edited