Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature add support for env variables #286

Merged
merged 10 commits into from
Oct 6, 2022
Merged

Feature add support for env variables #286

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
32f99c5
define root ar definitions.py
Shending-Help Oct 2, 2022
27a0159
file detector, parser and setting vars
Shending-Help Oct 2, 2022
81581f6
Delete hello_robyn.py
Shending-Help Oct 2, 2022
8d177a2
Delete configuration.conf
Shending-Help Oct 2, 2022
aea9a24
name, functions are now testable, var overrides?
Shending-Help Oct 3, 2022
dbf9297
testing files
Shending-Help Oct 3, 2022
c8d4005
pathlib instead of os, moving consts, logging
Shending-Help Oct 4, 2022
270cc39
calling the load_vars() automatically, checking for robyn.env first
Shending-Help Oct 5, 2022
7dc9975
fixing the load_vars() call
Shending-Help Oct 5, 2022
1ec134f
calling the load_vars() function in the constructor
Shending-Help Oct 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions definitions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import os

# Path to the root of the project
ROOT_DIR = os.path.dirname(os.path.abspath(__file__))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# Path to the environment variables
CONFIG_PATH = os.path.join(ROOT_DIR, 'configuration.conf')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I convention for the configuration file that I had in my mind was something like robyn.env. What do you think about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think robyn.env is great i've changed it to that

27 changes: 27 additions & 0 deletions robyn/import_vars.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import os
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

from definitions import CONFIG_PATH

# parse the configuration file returning a list of tuples (key, value) containing the environment variables
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('=')

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


# check for the environment variables set in cli and if not set them

def load_vars(variables = parser()):
"""Main function"""
for var in variables:
if var[0] in os.environ:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we would want to override the existing environment variables or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@sansyrox sansyrox Oct 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, makes sense. ✨
We can set a convention accordingly.

continue
else:
os.environ[var[0]]=var[1]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something similar for this function

def load_variables(variables = None):
    variables = parser() if variables is None
    ...