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

Merge with the environs library? #12

Closed
sloria opened this issue Apr 26, 2016 · 6 comments
Closed

Merge with the environs library? #12

sloria opened this issue Apr 26, 2016 · 6 comments

Comments

@sloria
Copy link
Contributor

sloria commented Apr 26, 2016

Thank you for envparse. I liked its API so much that I implemented a successor library, environs, which uses marshmallow to do the heavy lifting for casting, validation, and serialization.

environ's API is near-identical to envparse, with a few differences:

environs was mostly written as a proof-of-concept, though at this point it is stable enough to use (it's only ~100SLOC after all).

I wonder if we could reduce duplication of effort by merging these two libraries. I haven't yet thought about the particulars; just wanted to open the table for discussion first.

Thoughts?

@rconradharris
Copy link
Owner

rconradharris commented Apr 26, 2016

@sloria This looks really neat! First time seeing marshmallow and just glanced at environs really quickly, but wanted to give some initial thoughts before diving in more later:

  • LOVE the idea of separating out the type-parsing logic (and bonus that the library already exists!)
  • LOVE that extensibility is a focus from the beginning
  • LOVE that redundant APIs are ditched in favor of One Right Way(tm)
  • LOVE that preprocessor and postprocesser are ditched in favor of general purpose parsing
  • MIXED: Though it's at odds with the redundant API goal, the schema concept has proven useful in my usage of envparse at least. I like that I can declare a type and default once, and then reference it later in the code and have it do the right thing. It means that if I change a default or a type from a float to a decimal, I don't necessarily need to touch each place that referenced it. I would miss that feature if it went away. Maybe there's a more elegant way to handle it (?)
  • Proxied variables. These have been super useful for building apps on Heroku. I know you just haven't gotten around to adding it yet, but just wanted to reiterate how important they've been. I've switched from MailChimp to Mandrill back to MailChimp for email providers with minimal hassle because of the proxied variables abstracting out the environment variables exposed by plugins.
  • ON MERGING: This might be a case where environs could just win out. If its promise of a cleaner API is borne out, then there might not be a reason to continue envparse at all, just let it die on the vine so to speak. (I'm not that sentimental about it, it's served its purpose, so if something better comes along, I'm happy to switch :-)

@sloria
Copy link
Contributor Author

sloria commented Apr 26, 2016

Thank you for your feedback.

MIXED: Though it's at odds with the redundant API goal, the schema concept has proven useful in my usage of envparse at least. I like that I can declare a type and default once, and then reference it later in the code and have it do the right thing.

So, do you use your Env objects directly in your application code? As in

if env('FEATURE_X_ENABLE'):
    do_x()

vs.

feat_x_enabled = env.bool('FEATURE_X_ENABLED')
#...

if feat_x_enabled:
    do_x()

In the former case, the schema API makes a little more sense. I'm still unsure if it justifies the added API surface. I'll open an issue for discussion.

Proxied variables. These have been super useful for building apps on Heroku. I know you just haven't gotten around to adding it yet, but just wanted to reiterate how important they've been.

Yep, we'll definitely have proxied variables in environs 1.0.

ON MERGING: ...

OK, sounds likes a plan. I've made you a collaborator on environs. No pressure at all to do anything with it; just wanted to open the door for collaboration.

@rconradharris
Copy link
Owner

Thanks for the response!

Yeah I typically use the first-style. But perhaps the second-way really is the best so maybe the schema is unnecessary.

With that in mind, maybe the __call__ style should be abandoned since it duplicates env.get()?

@sloria
Copy link
Contributor Author

sloria commented Apr 28, 2016

maybe the __call__ style should be abandoned since it duplicates env.get()

Perhaps, but I quite like the conciseness of env('FOO'). If I had to choose between the two, I'd keep __call__. That said, I don't feel strongly against removing either.

@rconradharris
Copy link
Owner

Agree, either way or even keeping both is fine. It'll still be a lot cleaner than before.

@sloria
Copy link
Contributor Author

sloria commented Apr 30, 2016

Closing this. Thanks again for the feedback!

Btw, I've released environs 1.0, which adds support for proxied variables.

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

No branches or pull requests

3 participants
@rconradharris @sloria and others