search

gobuffalo / envy

Envy makes working with ENV variables in Go trivial.
MIT License
156 stars 21 forks source link

Adding a GetInt function to envy #4

Closed arschles closed 6 years ago

arschles commented 6 years ago

Is this ok to merge?

robbyoconnor commented 6 years ago

I can't merge it -- I just said it looked okay...

arschles commented 6 years ago

Ah got it. Sorry for the mixup, and thank you for reviewing it!

Sorry about my wording - I certainly didnt mean to come off rude. Cheers :)

On Mon, Mar 19, 2018 at 18:10 robbyoconnor notifications@github.com wrote:

I can't merge it -- I just said it looked okay...

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/gobuffalo/envy/pull/4#issuecomment-374438112, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEU0QcFTp1PjWKVwRgu84vl7C6DY_jhks5tgFbzgaJpZM4Rv2HL .

robbyoconnor commented 6 years ago

No issues!

markbates commented 6 years ago

TBH, I'm not a fan of this PR, sorry. Let me explain why.

First, the first part of the method is something that envy.Get already does, so all that GetInt really does is save 1 error check in someone's code. Not sure it's worth it, tbh.

Plus, if we add GetInt, then what about all the other types? What about GetBool, GetFloat, etc... I think you start to go down a big rabbit hole by introducing this method.

In Buffalo, for example, I originally had a ParamInt function on the context, that did a very similar thing, eventually it was ripped out for the reasons above.

Thank you for the time spent on this PR, and hopefully, I haven't discouraged you from submitting another one.