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.

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

Check the version of any of the supported Psycopg2 packages #178

Merged
merged 5 commits into from
Oct 9, 2019
Merged

Check the version of any of the supported Psycopg2 packages #178

merged 5 commits into from
Oct 9, 2019

Conversation

Tenzer
Copy link
Contributor

@Tenzer Tenzer commented Oct 9, 2019

A check was introduced in commit 8e0c485 which
would check what version of the 'psycopg2' Python (pip) package was installed
as the dependency was removed from setup.py.

The check would however only check the 'psycopg2' package and not the other two
supported providers of the psycopg2 module, which meant importing the
sqlalchemy_redshift module would throw an exception, even though they were
installed.

This changes the check to check for either of the three supported psycopg2
packages and throws an exception if any of them fail to validate.

I've also moved a comment back to setup.py about the required SQLAlchemy
version, which seemed to have been moved inadvertently in the same commit.

Todos

  • MIT compatible
  • Tests
  • Documentation
  • Updated CHANGES.rst

It was inadvertently moved in commit 8e0c485
to a place where it didn't make sense.
A check was introduced in commit 8e0c485 which
would check what version of the 'psycopg2' Python (pip) package was installed
as the dependency was removed from setup.py.

The check would however only check the 'psycopg2' package and not the other two
supported providers of the psycopg2 module, which meant importing the
sqlalchemy_redshift module would throw an exception, even though they were
installed.

This changes the check to check for either of the three supported psycopg2
packages and throws an exception if any of them fail to validate.
This syntax is only supported in Python 3.3 and up and is causing tests in
Python 2.7 to fail.
Copy link
Member

@jklukas jklukas left a comment

Choose a reason for hiding this comment

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

Overall, this looks great. Just one change I'd like to see, and then I think it's worth going ahead and publishing a new release to get make this fix available.

'A module was found named psycopg2, '
'but the version of it could not be checked '
'as it was neither the Python package psycopg2, '
'psycopg2-binary or psycopg2cffi.'
Copy link
Member

Choose a reason for hiding this comment

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

It seems unlikely, but if someone wants to import some unknown variant of psycopg2 besides these three, I'd prefer to pass here rather than raising an import error. Or perhaps best would be to log a warning message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I worked with the goal of extending the existing check to also cover the other two psycopg2 packages, and thought it would be good to still give an error in case any of the three packages weren't found.

I've removed the exception now so the version is only checked if any of the packages are found.

Copy link
Member

Choose a reason for hiding this comment

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

thought it would be good to still give an error in case any of the three packages weren't found

In my mind, that scenario should be covered by the import psycopg2 statement; it that doesn't raise an error, then the user must have installed some package intended to provide psycopg2 functionality.

Thanks for making the change.

@jklukas jklukas merged commit 53c6c48 into sqlalchemy-redshift:master Oct 9, 2019
@jklukas
Copy link
Member

jklukas commented Oct 9, 2019

I'll make sure the integration tests are passing, and then I'll make a new release. Feel free to bug me here if I lose track of this and don't make a new release by the end of the day.

@jklukas
Copy link
Member

jklukas commented Oct 9, 2019

0.7.5 is released.

@Tenzer
Copy link
Contributor Author

Tenzer commented Oct 9, 2019

Thank you for releasing this so quickly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants