-
-
Notifications
You must be signed in to change notification settings - Fork 323
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
Controlled casting #127
Controlled casting #127
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mathemancer I added some comments but I'm approving to unblock merge. If I'm around when you're done making changes, feel free to ask me to rereview before merge.
|
||
|
||
def get_supported_alter_column_types(engine): | ||
dialect_types = engine.dialect.ischema_names |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Datetimes should be fairly simple to add too, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather leave those for a separate issue / PR, since there'll be some complexity around dates vs times vs datetimes, and TZs (how to decide whether a column of times should be imported with the local TZ, assuming UTC, etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we've discussed the plan w.r.t. TZs, but I think having dates/times/datetimes separated out in a well-documented PR might benefit future code archaeologists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
create_varchar_casts(engine) | ||
|
||
|
||
def create_boolean_casts(engine): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you document what this function (and other similar functions) are doing in comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I documented them with docstrings to make it visible using the help
function. I also considered documenting them in comments in the PL/pgSQL
functions themselves instead, but I think folks are more likely to be able to (or want to) read the python docstrings than comments in the SQL. Do you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, agreed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, @mathemancer
|
||
|
||
def get_supported_alter_column_types(engine): | ||
dialect_types = engine.dialect.ischema_names |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
Fixes #126
This adds a number of DB functions, and replaces the column alteration logic to use them. These functions give us more precise control over what is done when casting one type to another (mostly to make things safer).
Technical details
This requires knocking out your Mathesar DB and reinstalling, since we still don't have upgrade functionality working.
Checklist
Update index.md
).master
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin