search

ni / nisystemlink-clients-python

Python API for interacting with a SystemLink Server, created and supported by NI.
https://ni.com/systemlink
MIT License
10 stars 14 forks source link

fix: Correct spelling for model class TableMetadataModification #44

Closed kjohn1922 closed 1 year ago

kjohn1922 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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