Feature add support for env variables #286
Conversation
- parse and set in import_vars
✅ Deploy Preview for robyn canceled.
|
There was a problem hiding this comment.
Hi @Shending-Help ,
Great work with the PR! ✨
I have added a few inline comments.
I would like to add an integration test in this PR, but we can address that once the comments have been addressed.
Great work again! 😄
robyn/import_vars.py
Outdated
| def parser(): | ||
| """Parse the configuration file""" | ||
| with open(CONFIG_PATH, 'r') as f: | ||
| for line in f: | ||
| if line.startswith('#'): | ||
| continue | ||
| yield line.strip().split('=') | ||
|
|
There was a problem hiding this comment.
I would recommend writing the function as the following
def parser(config_path=CONFIG_PATH):
...Declaring a function like this will allow it to be more testable and not have us mock the variables if we need to write any unit tests.
This is called reducing coupling. You can have a look at this concept here: https://www.youtube.com/watch?v=eiDyK_ofPPM
robyn/import_vars.py
Outdated
| def load_vars(variables = parser()): | ||
| """Main function""" | ||
| for var in variables: | ||
| if var[0] in os.environ: | ||
| continue | ||
| else: | ||
| os.environ[var[0]]=var[1] |
There was a problem hiding this comment.
Something similar for this function
def load_variables(variables = None):
variables = parser() if variables is None
...
robyn/import_vars.py
Outdated
| def load_vars(variables = parser()): | ||
| """Main function""" | ||
| for var in variables: | ||
| if var[0] in os.environ: |
There was a problem hiding this comment.
Do you think we would want to override the existing environment variables or not?
There was a problem hiding this comment.
I thought the priority is for environment variables that are set in the terminal or manually by the user not the ones that are in the config file so i made the code check for variables that already exist and not update those
There was a problem hiding this comment.
Okay, makes sense. ✨
We can set a convention accordingly.
robyn/import_vars.py
Outdated
| @@ -0,0 +1,27 @@ | |||
| import os | |||
There was a problem hiding this comment.
Maybe name this file something like environment_populator or env_populator? Or something similar according to you.
As a good practice is to keep the namespace or filename in accordance to what it does. And I assume the purpose of this file is to have helper functions for environment population.
definitions.py
Outdated
| import os | ||
|
|
||
| # Path to the root of the project | ||
| ROOT_DIR = os.path.dirname(os.path.abspath(__file__)) | ||
|
|
There was a problem hiding this comment.
We can either move the contents of the file to import_vars only or if you like to keep the constants in a separate file, we can create a submodule.
Let me know if you need any help with that.
There was a problem hiding this comment.
the idea was that this file defines where the root of the project is, because if i tried to define the root in a nested file it would be dependent on the path of the file I guess
There was a problem hiding this comment.
We can use Pathlib(https://docs.python.org/3/library/pathlib.html) to resolve the path. And I feel that we can move these constants and env_populator to a module of their own as the constants are only being used in the env_populator.py.
robyn/env_populator.py
Outdated
|
|
||
| for var in variables: | ||
| if var[0] in os.environ: | ||
| print("Variable {} already set".format(var[0])) |
There was a problem hiding this comment.
We can use logger module here. Similar to (https://github.com/sansyrox/robyn/blob/main/robyn/__init__.py#L21) .
We will be able to control the logs using the log-level.
Second, instead of format, we can use f-strings as we support Python >=3.7. https://docs.python.org/3/tutorial/inputoutput.html#tut-f-strings
There was a problem hiding this comment.
should logs be in debug level or info level? and I changed the syntax such that f-strings are used instead
There was a problem hiding this comment.
@Shending-Help , these logs should be debug level in my opinion.
There was a problem hiding this comment.
Or info works too. There is a very thin line and doesn't really matter that much.
definitions.py
Outdated
| ROOT_DIR = os.path.dirname(os.path.abspath(__file__)) | ||
|
|
||
| # Path to the environment variables | ||
| CONFIG_PATH = os.path.join(ROOT_DIR, 'configuration.conf') |
There was a problem hiding this comment.
I convention for the configuration file that I had in my mind was something like robyn.env. What do you think about it?
There was a problem hiding this comment.
i think robyn.env is great i've changed it to that
|
@Shending-Help , great job on the update! 😄 I have some more inline comments. In addition to them, I was also thinking of an integration test to verify the working. But we can do that once the changes have been merged. Thanks again! 😄 |
robyn/__init__.py
Outdated
| @@ -39,7 +40,7 @@ def __init__(self, file_object: str) -> None: | |||
| self.headers = [] | |||
| self.directories = [] | |||
| self.event_handlers = {} | |||
|
|
|||
| self.load_var = load_vars() | |||
There was a problem hiding this comment.
One last thing before we can proceed with the tests, self.load_var is None. So, we can just call the load_vars() function.
There was a problem hiding this comment.
okay I changed that
|
All looks good. 😃 Just need a test now |
Description
there are two files here
definitions.py => that file's whole purpose is to define where the root of the project is so we can find the config files placed there and access them even in nested files
import_vars.py => which parses the content of the configuration file and then check for overrides and if not it sets the env vars that can be imported and accessed any where
This PR fixes #97