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

Adds @moduledoc and @doc blocks to functions, including ## Examples #40

Closed
wants to merge 4 commits into from

Conversation

fireproofsocks
Copy link

@fireproofsocks fireproofsocks commented Apr 1, 2021

The big takeaway here is that your package IS compatible with releases -- the documentation just needs to clarify that.

This PR attempts to add more documentation per #39

  • Adds ## Examples sections to many functions
  • Shows a usage example of how to set this up to be compatible with releases
  • Removes confusing references to "serverless" -- despite invoking an unrelated technology, how values are stored (or even if they are stored at all) is an implementation detail, and not one that needs to be documented because it is "behind the interface".

@@ -44,26 +32,7 @@ Fetch your dependencies with `mix deps.get`.

Now, when you load your app in a console with `iex -S mix`, your environment variables will be set automatically.

#### Using Environment Variables in Configuration
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you remove this line? I think this is a helpful guide to using this in prod that doesn't seem to be reproduced in the other documentation you added.

Copy link
Author

Choose a reason for hiding this comment

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

😬 Eeep -- I confess that removal wasn't entirely intentional (attempting to work at home through too many distractions); the overall push was to point to the module docs for more info.

However, the important thing I was trying to get at is the method by which the variables are loaded. E.g. parsing .env files or running Mix.Task.run("loadconfig") from inside your application.ex should be unnecessary: by the time code in your application.ex is executed, the config ship has sailed. The config has already loaded and any code in them has already been executed (that's true for regular mix or for release execution). It's a simpler tie-in without the concerns re mix vs. releases or GenServers if the .env files are parsed in the config files (likely in runtime.exs). The examples outlined in the "Usage Suggestion" in the moduledoc covered the strategy that I think is the more effective one.

To illustrate one example, let's say I wanted to use System.fetch_env!/1 in my configs to enforce that a specific env variable had to be set. If I am loading an .env file (and setting system envs) in application start, it's too late: the app can never actually start unless I happen to have manually set that ENV elsewhere because System.fetch_env!/1 will raise an error before code in application.ex gets a chance to run. In order to work around loading the .env inside of the application start, I would have to soften up any reading of system env vars in my config files by using System.get_env/2 instead, which weakens the contract.

This starts to touch on what I wanted to run by you with a PR for issue 43.

I should say that any of these ideas are just fodder for discussion -- no worries if they don't seem like a good fit.

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