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

fix: Correct spelling for model class TableMetadataModification #44

Merged
merged 4 commits into from
Mar 8, 2023

Conversation

kjohn1922
Copy link
Contributor

What does this Pull Request accomplish?

TableMetdataModification -> TableMetadataModification

Why should this Pull Request be merged?

Correcting spelling of a model class.

What testing has been done?

Ran tests locally.

@spanglerco
Copy link
Collaborator

spanglerco commented Mar 7, 2023

Isn't this a breaking change needing a major version bump? I suppose alternatively we could have keep the old version around, documented to be deprecated, and change the typing everywhere to be Union[TableMetdataModification, TableMetadataModification] to indicate either class may be used. What do you think @mure?

Edit: Or change TableMetdataModification to subclass from TableMetadataModification and then change the typing to the correctly named one?

@kjohn1922
Copy link
Contributor Author

Isn't this a breaking change needing a major version bump? I suppose alternatively we could have keep the old version around, documented to be deprecated, and change the typing everywhere to be Union[TableMetdataModification, TableMetadataModification] to indicate either class may be used. What do you think @mure?

Edit: Or change TableMetdataModification to subclass from TableMetadataModification and then change the typing to the correctly named one?

I was planning on asking some questions about that. It does constitute a breaking change, and I'm not sure of the best practice for handling that. Also interested in @mure 's opinion.

@mure
Copy link
Contributor

mure commented Mar 7, 2023

Oof, that's pretty bad 🤦‍♂️
Yes, it would technically be a breaking change, but I would prefer not to pollute the types to fix the naming in a backwards compatible way.

That being said, I don't think anyone has really starting using the client yet, and it's even more unlikely that someone was calling the modify tables method. This is why I was hesitant to rush out the 1.0 release :/

Let me do some tinkering to see what our options are.

@mure
Copy link
Contributor

mure commented Mar 7, 2023

I think we can just add this line to the bottom of models/__init__.py:

# Alias to provide backwards compatibility for misnamed class, fixed in 1.0.2
TableMetdataModification = TableMetadataModification

kjohn1922 and others added 2 commits March 7, 2023 15:46
Co-authored-by: Paul Spangler <spanglerco@users.noreply.github.com>
@kjohn1922 kjohn1922 merged commit 30c760c into master Mar 8, 2023
@kjohn1922 kjohn1922 deleted the users/kelseyj/spelling branch March 8, 2023 14:05
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.

4 participants