Skip to content

Fix MyPy issues#533

Merged
georgesittas merged 2 commits intomainfrom
jo/fix_mypy
Mar 15, 2023
Merged

Fix MyPy issues#533
georgesittas merged 2 commits intomainfrom
jo/fix_mypy

Conversation

@georgesittas
Copy link
Contributor

Addressed mypy issues caused by recent SQLGlot commit

Copy link
Contributor

@crericha crericha Mar 15, 2023

Choose a reason for hiding this comment

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

Is there a reason properties are required? Does it make more sense to leave as is and in exp.update do properties or {}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think "properties" are required in the SQL UPDATE clause, right?

Copy link
Contributor

@crericha crericha Mar 15, 2023

Choose a reason for hiding this comment

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

I mean does update_table need to require them or can we keep as None and use an empty hash for update in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since update_table represents the execution of an UPDATE clause, I think it's logical to require them. I can't think of or find a counterexample in our codebase where it needs to have this flexibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. I wonder why it was like that in the first place then.

@georgesittas georgesittas merged commit ebb8121 into main Mar 15, 2023
@georgesittas georgesittas deleted the jo/fix_mypy branch March 15, 2023 17:07
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.

3 participants