Skip to content

Infer type from default if provided but type omitted#3

Open
BartoszCki wants to merge 1 commit into
Va1:masterfrom
BartoszCki:master
Open

Infer type from default if provided but type omitted#3
BartoszCki wants to merge 1 commit into
Va1:masterfrom
BartoszCki:master

Conversation

@BartoszCki

Copy link
Copy Markdown

This change proposition adds inferring type from default value. It does that only when type keyword argument was not provided. Proper tests were added.

Oh, and dropped using .pop as it seems just unnecessary.

Feel free to pick only some part of this, or none, no pressure ;)

Cheers

PS: I'm not sure if I should update version..

@mdxs

mdxs commented Mar 8, 2019

Copy link
Copy Markdown

It might also be an idea to allow interpretation of the default value.
By replacing the current:

  if value is None:
    if default_value is None:
      return None
    else:
      return default_value

with:

  if value is None:
    if default_value is None:
      return None
    value = default_value

Which will then be further evaluated/converted to the desired type.

Allowing (when LOST is not set as an environment variable):

>>> getenv('LOST', type=dict, default='{1: 2}')
{1: 2}

Then, for some more corner cases, also change:

  if desired_type is dict:
    return dict(literal_eval(value))

into:

  if desired_type is dict:
    return dict(literal_eval(str(value)))

As that will allow:

>>> getenv('LOST', default={'a': 3})
{'a': 3}

And... in that case (using a dict as default) would also works when the environment value is set to something that can be parsed into a dictionary.

@mdxs

mdxs commented Mar 8, 2019

Copy link
Copy Markdown

Then there are some corner cases to consider... as Python evaluates bool([]), bool({}), and bool(None) as False, it might be correct to change:

  if desired_type is bool:
    if value.lower() in ['false', '0']:
      return False
    else:
      return bool(value)

into:

  if desired_type is bool:
    if str(value).lower() in ['false', '0', '[]', '{}', 'none']:
      return False
    return bool(value)

Leaving yet another corner case... as getenv('LOST', type=bool, default=None) gets a shortcut when LOST is not set as an environment variable. Which requires the earlier:

  if value is None:
    if default_value is None:
      return None
    value = default_value

to be changed to:

  if value is None:
    if default_value is None:
      if desired_type is bool:
        return False
      return None
    value = default_value

@mdxs

mdxs commented Mar 8, 2019

Copy link
Copy Markdown

To summarize my proposals into one:

import os
from ast import literal_eval


__version__ = '1.2.1'


def getenv(name, **kwargs):
  """
  Retrieves environment variable by name and casts the value to desired type.
  If desired type is list or tuple - uses separator to split the value.
  """
  default_value = kwargs.get('default', None)
  list_separator = kwargs.get('separator', ',')
  desired_type = kwargs.get('type')
  if not desired_type:
    desired_type = type(default_value) if (default_value is not None) else str

  value = os.getenv(name, None)

  if value is None:
    if default_value is None:
      if desired_type is bool:
        return False
      return None
    value = default_value

  if desired_type is bool:
    if str(value).lower() in ['false', '0', '[]', '{}', 'none']:
      return False
    return bool(value)

  if desired_type in (list, tuple):
    value = value.split(list_separator)
    return desired_type(value)

  if desired_type is dict:
    return dict(literal_eval(str(value)))

  return desired_type(value)

But please note that I've not adjusted (or even run) the tests.

Thanks to the PR author and the original code for the concept and I hope my comments help you or anyone else to use the idea. As the PR author also said:

Feel free to pick only some part of this, or none, no pressure ;)

that also applies for my suggestions.

@BartoszCki

Copy link
Copy Markdown
Author

Thanks for the ideas @mdxs , they seem very reasonable!

I can't dig into in right now, but i definitely will do it soon

@mdxs

mdxs commented Mar 10, 2019

Copy link
Copy Markdown

No worries, here is an even further updated version; as I noted that tuple and list defaults when specified in their normal Python expression could cause issues. Therefore there is a test added to see if the then value type is not already a tuple or list type (if it is, we don't need to split a string by separator; something that fails when done to something already tuple/list).

# coding: utf-8

import os
from ast import literal_eval


__version__ = '1.2.2'


def getenv(name, **kwargs):
  """
  Retrieves environment variable by name and casts the value to desired type.
  If desired type is list or tuple - uses separator to split the value.
  """
  default_value = kwargs.get('default', None)
  list_separator = kwargs.get('separator', ',')
  desired_type = kwargs.get('type')
  if not desired_type:
    desired_type = type(default_value) if (default_value is not None) else str

  value = os.getenv(name, None)

  if value is None:
    if default_value is None:
      if desired_type is bool:
        return False
      return None
    value = default_value

  if desired_type is bool:
    if str(value).lower() in ['false', '0', '[]', '()', '{}', 'none']:
      return False
    return bool(value)

  if desired_type in (list, tuple):
    if type(value) not in (list, tuple):
      value = value.split(list_separator)
    return desired_type(value)

  if desired_type is dict:
    return dict(literal_eval(str(value)))

  return desired_type(value)

Again, please note that I didn't try the existing test cases, nor did I add new ones... sorry about that.

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