-
Notifications
You must be signed in to change notification settings - Fork 40
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.
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
chore: make psycopg2 optional #480
Conversation
@@ -67,7 +67,7 @@ def _check_content(filepath): | |||
assert CONTENT == f.read() | |||
|
|||
|
|||
root_dir = server_setup.DATA_ROOT_DIR | |||
root_dir = server_setup.DATA_ROOT_DIR.rstrip("/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a funny artifact of the config.ini
file I'm using with my other branch - it ends up overriding the DATA_ROOT_DIR
with LOCAL_DIR
, which is expected, but the trailing slash breaks a couple of tests here which use string concatenation to join paths for simplicity/readability. Real functionality uses os.path.join
so this isn't a problem elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You lost me. In your other branch, jon/windows
I presume, you have a config.ini
file where you set the path for the various variables, including DATA_ROOT_DIR
. But for what reason it is overwritten by LOCAL_DIR
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The config.ini sets the deployment mode, which results in using a LocalConfig
, where we default the DATA_ROOT_DIR
to be equal to LOCAL_DIR
since there is no need for separate locations. This caused a test failure since LOCAL_DIR
has an extra /
at the end. So this is just to make the tests work whether or not there is a config.ini causing this effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
Purpose
Simplify local installation by removing psycopg2 from requirements.txt and separate the code that depends on it, to prevent import errors if psycopg2 is not installed.
What the code is doing
pip install psycopg2
).Testing
tox
Time estimate
5 min