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

Ensure Date(time) fields are only converted to Nullable when they're not in the primary key #226

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dlouseiro
Copy link

As I reported in this issue, the fact that all Datetime fields are converted to Nullable caused synchronization issues when these fields are part of the table's primary key.

With this PR, I propose that this conversion to Nullable is only done when the field is not part of the table's primary key.

Implementation details:

  • Change definition of to_sql_type method not to convert Datetime fields to Nullable by default.
  • Create new method to_sql_type_non_primary_key method that ensures Datatime fields are converted to Nullable.
  • Change definition of create_empty_table to call to_sql_type_non_primary_key when the field is not part of the primary key, calling to_sql_type otherwise.

While there, formatted the code of connectors.py with ruff formatter to reduce the number of warnings/violations returned by pre-commit. It seems that a lot of other files are also not formatted, but decided to keep the number of non-functional code changes to a minimum.

The purpose of this PR is to make sure that primary key fields are never converted to `Nullable`, which is happening by default for `Date`, Datetime`, `Timestamp` and `Time` fields.
* Apply formatting

* Minimise number of changes
@dlouseiro
Copy link
Author

Hey @BTheunissen , as this is my first PR in this repo and I see you were the one introducing the nullification of datetime fields in this commit, would love your opinion and review here! Thank you

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.

1 participant