JakobGM / astrality

Astrality - A modular and dynamic configuration file manager
https://astrality.readthedocs.io
MIT License
98 stars 3 forks source link

Expand environment variables #19

Closed JakobGM closed 6 years ago

JakobGM commented 6 years ago

Before this change, you had to specify {{ env.EXAMPLE }} whenever you wanted to use an environment variable.

This PR adds the capability to add environment variables in the UNIX way ($EXAMPLE or ${EXAMPLE}) within any specified path and any specified string action parameter. Most users will end up trying this at some point, so we should support it.

You would still need to use the Jinja2 syntax whenever you want to use environment variables in YAML keys, but that should be really uncommon.

coveralls commented 6 years ago

Pull Request Test Coverage Report for Build 316


Totals Coverage Status
Change from base Build 312: 0.007%
Covered Lines: 3104
Relevant Lines: 3182

💛 - Coveralls
JakobGM commented 6 years ago

In config.py, is there a reason to convert it to str and then expandvars? I believe in python3, the function can take a Path object.

Yes, you are correct. I have changed it now. I was trusting an error message produced by mypy a bit too blindly. It was caused by a missing stub in typeshed (python/typeshed#1997). It will be fixed when python/typeshed#2053 is merged. For now I have added a # type: ignore comment.

Secondly, you can do path.expanduser() instead of Path.expanduser(path)

Fixed!

Lastly, can you not just do: path = os.path.expandvars(path.expanduser()) on line 267 instead of having the expandvars part later on?

I like to do this sort of stuff in seperate statements, as it becomes a bit more self-documenting, and the cost is likely minimal. But it is a loosely held opinion :stuck_out_tongue:

I will go ahead and merge the PR, thanks for the review!