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

Fix invalid SQLAlchemy version comparison #206

Merged
merged 2 commits into from Jun 30, 2020
Merged

Fix invalid SQLAlchemy version comparison #206

merged 2 commits into from Jun 30, 2020

Conversation

GrayAn
Copy link
Contributor

@GrayAn GrayAn commented Jun 29, 2020

This fix resolves a bug with invalid SQLAlchemy version comparison in the _get_column_info method (see discussion in the Issue 195). It includes packaging library as a dependency.

One question bothers me: packaging is released under the BSD-2 license, so I should mention it in this repository somehow. Should I add its licence to the LICENSE file?

Todos

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

@jklukas
Copy link
Member

jklukas commented Jun 29, 2020

packaging is released under the BSD-2 license, so I should mention it in this repository somehow

I don't have a great sense of the requirements in the BSD-2 and Apache licenses, but I would expect that nothing is strictly required here since we're only mentioning packaging as a dependency rather than redistributing its code. We publish wheels, etc. that contain only the sqlalchemy-redshift code and it is up to pip or other dependency management software to fetch dependencies like packaging.

It's entirely possible that I am wrong or that I'm missing perspective about common practice.

@jklukas
Copy link
Member

jklukas commented Jun 29, 2020

This change looks good and I'm happy to go ahead and merge it. Let me know if you'd like me to hold off, though, if you want to do some more investigation about altering our LICENSE.

@GrayAn
Copy link
Contributor Author

GrayAn commented Jun 30, 2020

Great! I think I'm overreacting to this license issue. I checked several popular python packages and it seems it works just like you described. So the PR is ready to be merged I think.

@jklukas jklukas merged commit c5f185b into sqlalchemy-redshift:master Jun 30, 2020
@jklukas
Copy link
Member

jklukas commented Jun 30, 2020

0.8.0 is now published, including this change.

@GrayAn GrayAn deleted the version_comparison branch July 2, 2020 06:17
@GrayAn
Copy link
Contributor Author

GrayAn commented Jul 2, 2020

Great, many thanks!

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