Skip to content

Conversation

erikced
Copy link
Contributor

@erikced erikced commented Sep 29, 2017

Change usage of SQLAlchemy's get_primary_keys to get_pk_constraint which
returns a dict with constrained_columns containing the primary key
fields and optionally name with the name of the pk constraint.
Update get_primary_keys_info to compare the entire dicts and apply
ignores on the constraint name rather than the key names if a name
field exists. If name does not exist, the primary key columns are
compared instead.

@mattbennett
Copy link
Collaborator

@erikced thanks for the PR! Sorry to leave this PR hanging for such a long time.

I've just updated the travis configuration so the tests run on their new infrastructure. Do you want to merge master into this branch so the tests run here?

@erikced erikced closed this Nov 19, 2017
@erikced erikced reopened this Nov 19, 2017
Change usage of SQLAlchemy's get_primary_keys to get_pk_constraint which
returns a dict with 'constrained_columns' containing the primary key
fields and optionally 'name' with the name of the pk constraint.
Update get_primary_keys_info to compare the entire dicts and apply
ignores on the constraint name rather than the key names if a 'name'
field exists. If 'name' does not exist, the primary key columns are
compared instead.
@erikced erikced force-pushed the primary-key-constraint-comparison branch from 257d617 to fdd2cf2 Compare November 19, 2017 22:02
@erikced
Copy link
Contributor Author

erikced commented Nov 20, 2017

@mattbennett done.

Unfortunately MySQL/MariaDB does not appear to support constraint names for primary keys so the end-to-end-tests only test the fallback to the current behaviour.

@mattbennett mattbennett requested a review from timbu November 27, 2017 10:10
Copy link
Collaborator

@mattbennett mattbennett left a comment

Choose a reason for hiding this comment

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

Thanks @erikced! I will release this in the next cut.

For the end-to-end tests we can think about adding Postgres or a similar alternative RDBMS, but that can come as a later update

@mattbennett mattbennett merged commit b46282a into gianchub:master Jan 25, 2018
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.

2 participants