coverage: 96.957% (-0.07%) from 97.025%
when pulling 84b9ecb on sheinbergon:master
into 82251f3 on agronholm:master.

This needs a regression test and a changelog entry.

This needs a regression test and a changelog entry.

OK, I'll add the changes to the Changes.rst

As for a regression test - there's already a test that generates an enum (an intiial trial attempt caused it to fail).
However, like all of your tests, it doesn't really generate code from a live db, but rather from an SQLalchemy definition set,

so it was passing all along, while differing from real world behavior which caused a differen enum statement to be generated. How would you like to tackle that?

I can see if maybe a specific postgresql.ENUM dialect would trigger the error

This needs a regression test and a changelog entry.

OK, I'll add the changes to the Changes.rst

As for a regression test - there's already a test that generates an enum (an intiial trial attempt caused it to fail). However, like all of your tests, it doesn't really generate code from a live db, but rather from an SQLalchemy definition set,

so it was passing all along, while differing from real world behavior which caused a differen enum statement to be generated. How would you like to tackle that?

I can see if maybe a specific postgresql.ENUM dialect would trigger the error

Yes, that would probably do the trick. IIRC I've used this technique elsewhere in the test suite when the generic types weren't reproducing the issues I wanted to fix.

This needs a regression test and a changelog entry.

OK, I'll add the changes to the Changes.rst
As for a regression test - there's already a test that generates an enum (an intiial trial attempt caused it to fail). However, like all of your tests, it doesn't really generate code from a live db, but rather from an SQLalchemy definition set,
so it was passing all along, while differing from real world behavior which caused a differen enum statement to be generated. How would you like to tackle that?
I can see if maybe a specific postgresql.ENUM dialect would trigger the error

Yes, that would probably do the trick. IIRC I've used this technique elsewhere in the test suite when the generic types weren't reproducing the issues I wanted to fix.

@agronholm so, I took a shot at it.

This is not reproducible with the current set of unit tests (not for the tables, nor for the declarative generator).
Even if I define PostgreSQL ENUM dialect type. When the Table is rendered and metadata is being loaded with the column definitions, it always downgrades the dialect specific ENUM type to a plain SQLAlchemy Enum type, thus skipping this entire error.

I can confirm my fix solves the issues for both declarative and tables (as they both share the same function).

The only way I see to do a regression test for something like is by doing real integration / e2e tests against a dockerized PostgreSQL DB. But that's going to be a whole lot of work for a one line fix ( perhaps worth considering for the future)

So, adding a test brings 0 value ATM on top of the current "fancy_types" one. If you know of way to have the dialect specific ENUM type survive metadata reflection, please let me know.

If not, I suggest we merge this fix without adding a test.

Added changes to change log, as you requested.

This needs a regression test and a changelog entry.

OK, I'll add the changes to the Changes.rst
As for a regression test - there's already a test that generates an enum (an intiial trial attempt caused it to fail). However, like all of your tests, it doesn't really generate code from a live db, but rather from an SQLalchemy definition set,
so it was passing all along, while differing from real world behavior which caused a differen enum statement to be generated. How would you like to tackle that?
I can see if maybe a specific postgresql.ENUM dialect would trigger the error

Yes, that would probably do the trick. IIRC I've used this technique elsewhere in the test suite when the generic types weren't reproducing the issues I wanted to fix.

@agronholm so, I took a shot at it.

This is not reproducible with the current set of unit tests (not for the tables, nor for the declarative generator). Even if I define PostgreSQL ENUM dialect type. When the Table is rendered and metadata is being loaded with the column definitions, it always downgrades the dialect specific ENUM type to a plain SQLAlchemy Enum type, thus skipping this entire error.

I can confirm my fix solves the issues for both declarative and tables (as they both share the same function).

The only way I see to do a regression test for something like is by doing real integration / e2e tests against a dockerized PostgreSQL DB. But that's going to be a whole lot of work for a one line fix ( perhaps worth considering for the future)

So, adding a test brings 0 value ATM on top of the current "fancy_types" one. If you know of way to have the dialect specific ENUM type survive metadata reflection, please let me know.

If not, I suggest we merge this fix without adding a test.

Added changes to change log, as you requested.

Thanks. As for the dialect specific enum type, aren't we skipping reflection and creating a metadata directly in the test? So basically we're creating a metadata object as if it had come back from SQLAlchemy reflection. What is happening there then that is different from what happens with a real PostgreSQL server?

This needs a regression test and a changelog entry.

OK, I'll add the changes to the Changes.rst
As for a regression test - there's already a test that generates an enum (an intiial trial attempt caused it to fail). However, like all of your tests, it doesn't really generate code from a live db, but rather from an SQLalchemy definition set,
so it was passing all along, while differing from real world behavior which caused a differen enum statement to be generated. How would you like to tackle that?
I can see if maybe a specific postgresql.ENUM dialect would trigger the error

Yes, that would probably do the trick. IIRC I've used this technique elsewhere in the test suite when the generic types weren't reproducing the issues I wanted to fix.

@agronholm so, I took a shot at it.
This is not reproducible with the current set of unit tests (not for the tables, nor for the declarative generator). Even if I define PostgreSQL ENUM dialect type. When the Table is rendered and metadata is being loaded with the column definitions, it always downgrades the dialect specific ENUM type to a plain SQLAlchemy Enum type, thus skipping this entire error.
I can confirm my fix solves the issues for both declarative and tables (as they both share the same function).
The only way I see to do a regression test for something like is by doing real integration / e2e tests against a dockerized PostgreSQL DB. But that's going to be a whole lot of work for a one line fix ( perhaps worth considering for the future)
So, adding a test brings 0 value ATM on top of the current "fancy_types" one. If you know of way to have the dialect specific ENUM type survive metadata reflection, please let me know.
If not, I suggest we merge this fix without adding a test.
Added changes to change log, as you requested.

Thanks. As for the dialect specific enum type, aren't we skipping reflection and creating a metadata directly in the test? So basically we're creating a metadata object as if it had come back from SQLAlchemy reflection. What is happening there then that is different from what happens with a real PostgreSQL server?

I have no idea about what might be different. But that's deep in SQLAlchemy territory. I think it's wrong to assume the test method starts from "here's a database reflected type", as it assumes to know a lot about SA internals, and you're always testing without running a crucial part of the real-world flow which actually connects to a database.

Again - technically speaking - this was already covered in a test, except for the fact that test was always false positive

The correct approach would be to add real world integration tests, those will NEVER provide false positives as long as we test against a wide range of RDBMSs and versions. I suggest we open another issue for that and move forward.