2 ms·

Nice post. One part that raised a red flag for me was: defp bucket!, do: System.fetch_env!("BUCKET_NAME") This will look at that environment variable at

Nice post. One part that raised a red flag for me was: defp bucket!, do: System.fetch_env!("BUCKET_NAME") This will look at that environment variable at build time. For many workflows this doesn't work because you may build the app in CI and run it with different buckets in production and staging. I always default to using Application.get_env/2 and setting the value in runtime.exs, now that that's a thing in recent Elixir. For things where performance matters in a hot loop, it might be worth compiling in the value, but I wouldn't imagine that's the case here where it's a part of a network call to AWS.

ilikehurdles 3y ago
This will look at it at runtime not build time. `System.fetch_env!("BUCKET_NAME")` is still in the body of the function and won't get executed until it's called after application start. It is still a good reason to use Application.get_env, and it should be preferred over peppering system environment variable fetches all over the application.
- losvedir 3y ago
  Oh, of course. Brain fart moment. I pattern matched (ha!) to code I've seen too much where that's at compile time. Yeah, runtime here works but I agree is still not as good as Application.get_env. Aside from (IMO) code clarity reasons, last I checked it is (or can be) much quicker. Application is a quick ETS lookup, while System reaches out to the system and I forget the details but at one point that was much slower.
thibaut_barrere 3y ago
As pointed out, it's not compile time but runtime here unless I'm missing anything. And it would be a good reason to build a Credo check exactly for this (I've done a bit of research about this). Any calls to System in regular compile time config (e.g. config/prod/dev/test or included) should be forbidden, same for attribute-level / meta code. I hope we will get this one day!
lawik 3y ago
There are a bunch of things that are kind of quick and dirty about these snippets. I think I mentioned that in the post, that these are not best practices. But not quite so ill as compile-time in this case :)

ilikehurdles 3y ago

This will look at it at runtime not build time. `System.fetch_env!("BUCKET_NAME")` is still in the body of the function and won't get executed until it's called after application start. It is still a good reason to use Application.get_env, and it should be preferred over peppering system environment variable fetches all over the application.

losvedir 3y ago
Oh, of course. Brain fart moment. I pattern matched (ha!) to code I've seen too much where that's at compile time. Yeah, runtime here works but I agree is still not as good as Application.get_env. Aside from (IMO) code clarity reasons, last I checked it is (or can be) much quicker. Application is a quick ETS lookup, while System reaches out to the system and I forget the details but at one point that was much slower.

thibaut_barrere 3y ago

As pointed out, it's not compile time but runtime here unless I'm missing anything. And it would be a good reason to build a Credo check exactly for this (I've done a bit of research about this). Any calls to System in regular compile time config (e.g. config/prod/dev/test or included) should be forbidden, same for attribute-level / meta code. I hope we will get this one day!

lawik 3y ago

There are a bunch of things that are kind of quick and dirty about these snippets. I think I mentioned that in the post, that these are not best practices. But not quite so ill as compile-time in this case :)