Better support for boolean values

[thilp]

When specifying true instead of True as a value for an environment variable typed as bool in Ecological, the program crashes with an obscure error.

Expect behavior:

• ideal: true (as well as TRUE and other likely variations) is handled identically to True (and similarly for False);
• acceptable: The error message warns that only valid Python expressions are accepted as values.

I don't know if this is or should be limited to boolean values.

Replay

$ python3 -m venv /tmp/venv
$ source /tmp/venv/bin/activate
(venv) $ python3 -m pip install ecological
...
Successfully installed ecological-1.6.0

# test.py
import ecological

class Config(ecological.AutoConfig, prefix="test"):
    hi: bool = False

print(f"Config.hi = {Config.hi!r}")

(venv) $ python3 test.py  # all is well
Config.hi = False
(venv) $ env TEST_HI=True python3 test.py  # all is well
Config.hi = True
(venv) $ env TEST_HI=true python3 test.py  # "true" instead of "True"
Traceback (most recent call last):
  File "/tmp/venv/lib/python3.7/site-packages/ecological/autoconfig.py", line 135, in get
    value = self.transform(raw_value, wanted_type)
  File "/tmp/venv/lib/python3.7/site-packages/ecological/autoconfig.py", line 90, in cast
    if isinstance(representation, str)
  File "…/.pyenv/versions/3.7.2/lib/python3.7/ast.py", line 91, in literal_eval
    return _convert(node_or_string)
  File "…/.pyenv/versions/3.7.2/lib/python3.7/ast.py", line 90, in _convert
    return _convert_signed_num(node)
  File "…/.pyenv/versions/3.7.2/lib/python3.7/ast.py", line 63, in _convert_signed_num
    return _convert_num(node)
  File "…/.pyenv/versions/3.7.2/lib/python3.7/ast.py", line 55, in _convert_num
    raise ValueError('malformed node or string: ' + repr(node))
ValueError: malformed node or string: <_ast.Name object at 0x7fbf363839e8>

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "test.py", line 3, in <module>
    class Config(ecological.AutoConfig, prefix="test"):
  File "/tmp/venv/lib/python3.7/site-packages/ecological/autoconfig.py", line 196, in __new__
    value = attribute.get(attribute_type)
  File "/tmp/venv/lib/python3.7/site-packages/ecological/autoconfig.py", line 137, in get
    raise ValueError(f"Invalid configuration for '{self.name}': {e}.")
ValueError: Invalid configuration for 'TEST_HI': malformed node or string: <_ast.Name object at 0x7fbf363839e8>.

[marcinzaremba]

Thanks for pointing it out. That's true that user-friendliness would be improved very much using such approach. However that would be the first time that ecological would try to invent its own parsing rules and language that is different from Python that I understand was one of original assumptions (straightforward and simple). Is it worth changing this approach?

[thilp]

If making exceptions (like I described in my "ideal" case above) is not on the table, would it still be possible to catch the exception from ast.literal_eval and display a better error?

[marcinzaremba]

If making exceptions (like I described in my "ideal" case above) is not on the table

Everything is on the table. That would just require significant change of the library's philosophy and I am just looking for additional reasoning behind that.

would it still be possible to catch the exception from ast.literal_eval and display a better error?

What would be the better error in this case?
Currently Ecological differentiates only between if something needs to be Python-evaluated or not and presented exception is the best that we have at the moment.

[marcinzaremba]

@thilp Can you provide an example of what error message you would expect here?

[thilp]

I think the most useful message in my case would have been something like:

• ValueError: Invalid configuration for 'TEST_HI': expected 'True' or 'False' but got 'true'
• ValueError: Invalid configuration for 'TEST_HI': value 'true' is not a valid Python expression

as it would have pointed me directly to the source of the error and proposed a fix. I would also have enjoyed a shorter stacktrace because I'm not interested in implementation details.

If making exceptions (like I described in my "ideal" case above) is not on the table

Everything is on the table. That would just require significant change of the library's philosophy and I am just looking for additional reasoning behind that.

Imho libraries should strike a balance between happy users and sustainable growth. It is totally fine if Ecological's philosophy is to only support syntactically-valid Python inputs, especially if that leads to a smaller, cleaner codebase, but then I think it restricts a bit its userbase as well: programs relying on it can only be operated by users that are aware of this unusual behavior (which, as far as they are concerned, is an implementation detail). It's not a bad thing, but it leaves an empty space to be filled by e.g. another library wrapping Ecological by translating non-Pythonic user inputs. If the necessary work is small and straightforward, I believe it's simpler (from a purely logistic point of view) to do it in Ecological than building on top of it.

[marcinzaremba]

I am convinced now. Sounds like a good addition. My proposal would be:

• go with Invalid configuration for 'TEST_HI': value 'true' is not a valid Python expression as it does not require much work (no need to provide user with a range of valid inputs for each type) and for sure improves experience
• provide new, more configurable transform function that can be configured to tolerate/handle things like:
  • true/false instead of True/False
  • one,two,three (comma-separated list) when Python List is a wanted type.

What do you think?

[thilp]

Cool! I think the cleaner error message is definitely a low-hanging and delicious fruit.

As discussed offline, I also think that transform is the right place to extend Ecological's parsing of input values. I don't know exactly what the final result should look like, but it should be extremely easy for developers to opt-in to the more tolerant parsing (if the set of inputs recognized by the "lax" parsing is a superset of the inputs recognized by the "Python-only" parsing, as I think it should be), and I would also recommend switching the default parsing to the "lax" one in the future, while still offering the possibility for the developer to specify a custom parsing.

[marcinzaremba]

@thilp Do you maybe have more ideas what this new parser should tolerate?

[thilp]

You mentioned lists like ab,cd,ef already, perhaps dicts on the same format (a:b,c:d,e:f)? But it might be simpler to start with a minimal set (e.g. only the booleans) and extending it according to what users actually ask for. I almost never needed dicts in an environment variable…