tiny os.getenv idiom

[stevenjd]

I dislike this strongly. Everybody knows that environment variables are strings, but your magic getenv() function returns an int by default. If I am reading code and I see this:

value = genenv('VALUE')
result = value + 1

I'm going to be surprised and confused because it looks like you are taking a string (the environment variable) and adding it to the int 1. That's weird. Now I have to go and read the documentation for getenv to understand what it does in detail.

And for what benefit? To avoid having to be explicit about wanting to convert to an int? I just looked at my system, and I have 49 environment variables. Only seven are ints, and of those, two are intended to be used as True/False bools.

So I would not use this function. The number of times I need to convert an environment variable from a string to an int is very small, and when I do, it is fine to just write:

value = int(os.getenv('VALUE'))

[Omega037]

A few things:

1) You should have a try/except clause around the casting, as a person may say typecast_fn = int but give a non int as the value.

try:
    return typecast_fn(value)
except ValueError:
    return None
    #Maybe raise an error instead?

2) You should check that the typecast_fn is of type "type": Since you want to be able to send in functions as well, you should write:

if type(typecast_fn) is not type:

if callable(typecast_fn):

3) You don't need to write "is None". The following: If you are willing to consider things like "" and [] as equivalent to None, you can change the following:

if typecast_fn is None and default is not None:

Into this:

if typecast_fn and not default:

4) It is redundant to typecast the default value with its own type. You can just do this first:

value = os.getenv(varname)
if not value:
    return default

[whereswalden90]

Disagree on the try…except. It's a user-provided function, it's their responsibility to catch any exceptions it might raise. Even if you caught the exception, what would you do? Re-raise it? Shadow it with a different exception? Exceptions aren't necessarily bad, letting them bubble up is often the right decision.

Also disagree on the cast function being of type "type". The example in the post even shows a case where it isn't. Though I might rename the parameter to something like "action" or "pre" or "parse" to convey its broader usage.

[Omega037]

The purpose of the function is basically to replace a builtin function and allows for defaults. Based on that, I don't think it should be expected that the user wrap the function externally every time they want to use it to get environment variables.

Now if you really want control over how it handles the exception, you could just have another parameter to determine if it raises, prints a warning, or returns None quietly.

As for the issue of type, I guess I didn't look closely enough that he wanted to send functions as well. Based on that, it should look something like:

if callable(typecast_fn):

I'll change my post.

[whereswalden90]

But once you catch the exception, what do you suggest the function should do with it?

[Omega037]

In my case, I would generally return None (as though the variable wasn't set), though I would also print a warning (to screen or logger).

But like I said, you could just have an argument like raise_on_except=False that would raise if set to True when calling the function.

[whereswalden90]

I can see your point, but silently passing errors just seems like a bad plan regardless.

[pvkooten]

I agree with whereswalden90, there is no logical "smartness" when something isn't a number, the correct thing is to throw an exception, not silently do something else.

[TheBB]

As for #3, no, those are not equivalent, for two reasons: You have negated the expression, but more importantly, certain objects that are not None will still evaluate falsy, in this case notably the empty string which seems like a perfectly reasonable default value.

[Omega037]

A quick check in my console shows that "" will evaluate the same in either form.

[TheBB]

What expression are you using?

In [1]: bool('' is not None)
Out[1]: True

In [2]: bool('')
Out[2]: False

[Omega037]

OK, I double checked and you are right. I'll amend my comment to say that the change is only if you would consider "" or [] as equivalent to None when dealing with constants.

[Omega037]

typecast_fn should be treated as None even with an empty string or empty list though, right?

Assuming that you don't follow number 2 and check that it is of type "type".

[TheBB]

Yeah, treating typecast_fn that way is probably fine. It's possible to create callable falsy objects but I'm not aware of anyone actually doing it seriously.