spandex-project / spandex

A platform agnostic tracing library
MIT License
335 stars 53 forks source link

Add warning about use of Mix.env/0 to README #77

Closed bitwalker closed 6 years ago

bitwalker commented 6 years ago

Using Mix.env/0 in the configuration example is likely to lead to issues when someone tries to use code like that in a release, as Mix is not intended (and basically does not work) in releases. This PR adds a warning and alternative option for the same config, but it may be preferable to remove the use of Mix.env/0 entirely and use something else as an example.

Nice work!

sourcelevel-bot[bot] commented 6 years ago

Hello, @bitwalker! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

coveralls commented 6 years ago

Pull Request Test Coverage Report for Build 339


Totals Coverage Status
Change from base Build 337: 0.0%
Covered Lines: 197
Relevant Lines: 240

💛 - Coveralls
zachdaniel commented 6 years ago

I think I agree that just entirely removing that example would be best. Perhaps setting it to some evaluation of a system variable, or something else to illustrate that it can be done dynamically. Feel free to add a better example to remove the need for the warning, or to just remove that section entirely and we can add an example afterwards.

Thanks for your contribution!

bitwalker commented 6 years ago

@zachdaniel Sure thing! I've updated the example to use System.get_env/1 instead, I think that covers the same purpose and doesn't carry the baggage of Mix.env/0.

zachdaniel commented 6 years ago

🎉

GregMefford commented 6 years ago

Thanks @bitwalker! ❤️