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

_get_column_info() missing 1 required positional argument: 'comment' in SQLAlchemy 1.2.0 #138

Closed
jleclanche opened this issue Dec 27, 2017 · 7 comments

Comments

@jleclanche
Copy link
Contributor

@jleclanche jleclanche commented Dec 27, 2017

column_info = super(RedshiftDialect, self)._get_column_info(

This breaks, because this was added:

http://docs.sqlalchemy.org/en/latest/changelog/changelog_12.html#change-3f38508932257c303017b1276f285c39

When it's called from get_columns():

column_info = self._get_column_info(
name=col.name, format_type=col.format_type,
default=col.default, notnull=col.notnull, domains=domains,
enums=[], schema=col.schema, encode=col.encode)

Because comment is missing from there.

@jleclanche
Copy link
Contributor Author

@jleclanche jleclanche commented Dec 27, 2017

@jleclanche
Copy link
Contributor Author

@jleclanche jleclanche commented Dec 27, 2017

Would it be crazy to add tests against sqlalchemy master branch to avoid getting caught up on short notice like this in the future?

@jklukas
Copy link
Collaborator

@jklukas jklukas commented Jan 4, 2018

Would it be crazy to add tests against sqlalchemy master branch to avoid getting caught up on short notice like this in the future?

I don't know that it would particularly help, since tests only get run when we new commits get sent to PRs. Months can go by without any activity here.

@jklukas
Copy link
Collaborator

@jklukas jklukas commented Jan 4, 2018

This looks potentially messy if we want to maintain compatibility with SQLA pre-1.2.0 since the new comment parameter is the postgres driver doesn't have a default. We'd need to check the sqlalchemy library version or catch an error to dispatch to a different signature.

@graingert - Do you have thoughts about whether we could make SQAlchemy 1.2.0 the minimum required version for the next release?

@jleclanche
Copy link
Contributor Author

@jleclanche jleclanche commented Jan 4, 2018

Months can go by without any activity here.

I think we can cron travis jobs nowadays.

jklukas added a commit that referenced this issue Jan 16, 2018
Addresses #138
@jklukas
Copy link
Collaborator

@jklukas jklukas commented Jan 16, 2018

For review: #139

jklukas added a commit that referenced this issue Jan 17, 2018
* Fix incompatibility of reflection with SQA 1.2.0

Addresses #138
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.