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

Add deprecated warning for Worksheet.update method #1209

Closed
lavigne958 opened this issue Jun 6, 2023 · 2 comments · Fixed by #1226
Closed

Add deprecated warning for Worksheet.update method #1209

lavigne958 opened this issue Jun 6, 2023 · 2 comments · Fixed by #1226
Assignees
Milestone

Comments

@lavigne958
Copy link
Collaborator

Overview

Add a new deprecation warning about the changes in the method signature:
it will now be: Worksheet.update(values: List[List[str]], range_name: Optional[str], ....)

This will ensure we receive the expected argument in the expected format and can be statically typed and checked.

Details

The method Worksheet.update has a very complicated format:

  • it can take a range followed by a list of values: `sheet.update("A1:B1", ["42", "43"])
  • it can take a range followed by a matrix of values: `sheet.update("A1:A2", [["52"], ["53"]])
  • it can take only values: `sheet.update(["64", "65"])
  • it can take only a matrix of values: `sheet.update([["75"], ["76"]])

This is complicated to type and for a good reason because it's complicated to use.

swap the 2 arguments and enforce the values to be a matrix:

  1. because the values are mandatory so they must come first
  2. the range is optional so it must come after the values
  3. The values are required to be a matrix, we can trick and create the matrix if not given but as this is what is required by the API we should expect it to be formatted this way.
  4. in order to provide static type hints we must enforce simple yet coherent types.

@alifeee what do you think about this ?

@lavigne958 lavigne958 added this to the 5.10 milestone Jun 6, 2023
@alifeee
Copy link
Collaborator

alifeee commented Jun 6, 2023

I like the new type signature, much more than the previous implementation. It will definitely be good to implement this method.

I am not fully up to date/opinionated about breaking changes, so, I believe I am right in thinking that either:

  • update is changed, meaning people must change their function calls to upgrade to v6.0.0
  • a new method (e.g., update_range) is created, so both methods can be used

I think that creating a new method would be pointless - if people want to keep their code the same they can stay on v5. (I imagine most people will not even consider upgrading, so it does not matter much to keep things very similar)

@lavigne958
Copy link
Collaborator Author

you're right I think we should update the method signature so people keep using the method and we keep the behavior in a single place (and this time well organized argument wise lol)

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 a pull request may close this issue.

2 participants