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

No migration path for Worksheet.update() #1360

Closed
NickCrews opened this issue Dec 4, 2023 · 14 comments
Closed

No migration path for Worksheet.update() #1360

NickCrews opened this issue Dec 4, 2023 · 14 comments
Assignees

Comments

@NickCrews
Copy link
Contributor

NickCrews commented Dec 4, 2023

I just updated to the newest version and am now getting the DeprecationWarning added in #1209

Before I was using the API of worksheet.update(data) to update every cell. However, if I now try worksheet.update(range_name=":", values=data), I get gspread.exceptions.APIError: {'code': 400, 'message': "Unable to parse range: 'Sheet1'!:", 'status': 'INVALID_ARGUMENT'}.

How do I migrate to maintain the past behavior? Before 6.0.0 is release can we make the API be

def format(self, values: list[list], *, range_name=...)

AKA values is required and can either be positional or kwarg, but the rest of the args are keyword only? If range_name is omitted, then it is assumed to be the entire worksheet?

I can submit PRs if you want, though I will be slower than you will be as I don't quite understand your use of the @accepted_kwargs decorator.

@alifeee
Copy link
Collaborator

alifeee commented Dec 4, 2023

Hi! Thanks for the issue :)

As I understand your problem, you want to change your code before v6.0.0 releases so that it will work with the new version. Valiant!

Before I get into the meat, I will tell you: you don't have to change anything. worksheet.update(data) will continue working just fine. It is a failure in our warning message to make that clear.

It is not very clear, as the warning says

DeprecationWarning: [Deprecated][in version 6.0.0]: Method signature's arguments 'range_name' and 'values' will change their order. We recommend using named arguments for minimal impact. In addition, the argument 'values' will be mandatory of type: 'List[List]'. (ex) Worksheet.update(values = [[]], range_name=)

but in v6 you can still use worksheet.update(data) just fine. In fact, this is the suggested way, as the signatures were

v5 signature

def update(
        self,
        range_name,  # required
        values=None, # optional
        **kwargs
    ):

v6 signature

def update(
        self,
        values: Iterable[Iterable[Any]],  # required
        range_name: Optional[str] = None, # optional
...

In v5, range_name is required. If values is not given, the call to update does nothing. This is pointless. And, you can give an array into range_name and it will pretend that array is instead values. This prompted the change by @lavigne958 in #1209.

In v6, values is required. If range_name is not given, it inserts the values starting at "A1". This makes sense. This is what you were doing already.

Changes

This issue being created tells me that the situation is unclear. We can solve it in a couple of ways

  • make the warning message & migration guide clearer (that you don't have to change your arguments to keyword-arguments if you don't specify a range)

In fact, in #1336 I added code that swaps the arguments if they "seem" the wrong way round:

gspread/gspread/worksheet.py

Lines 1215 to 1222 in 7974168

if isinstance(range_name, (list, tuple)) and isinstance(values, str):
warnings.warn(
"The order of arguments in worksheet.update() has changed. "
"Please pass values first and range_name second"
"or used named arguments (range_name=, values=)",
DeprecationWarning,
)
range_name, values = values, range_name

Thus, another option is:

  • We remove the deprecation warning. Users will begin seeing the above warning upon update to v6.0.0, and may or may not swap their argument order, or swap to kwargs. Either way, their code should continue working.

So, apologies for my waffle, but, in the end: you can continue without changing your code. We can either make the warning clearer (already done once... #1310) OR we can remove the warning (potentially dangerous if the above "catch" doesn't work in some edge cases).

Finally, I am afraid whatever you do you cannot disable the warning message that update emits. See #1312 for discussion on this topic.

p.s., worksheet.update(range_name=":", values=data) would also work, but I recommend sticking with worksheet.update(data)

@alifeee alifeee closed this as completed Dec 4, 2023
@alifeee
Copy link
Collaborator

alifeee commented Dec 4, 2023

p.p.s.,

I can submit PRs if you want, though I will be slower than you will be as I don't quite understand your use of the @accepted_kwargs decorator.

neither do we that is why we removed it in v6.0.0 haha

@alifeee alifeee self-assigned this Dec 4, 2023
@NickCrews
Copy link
Contributor Author

Ah, sorry I assumed master was where the 6.0.0 release was getting cut from. I didn't see the feature branch. Probably you have good reason, but if master was tip of tree where 6.0.0 was going to get released from, then I wouldn't be confused. So that is another change you could do

@NickCrews
Copy link
Contributor Author

yes, the 6.0.0 API looks great.

The migration guide in the readme is confusing, it should be changed from

- file.sheet1.update(["54"], "B2")
+ file.sheet1.update(range_name="I7", values=[["54"]])

to

- file.sheet1.update("B2", ["54"])
+ file.sheet1.update([["54"]], range_name="B2")
+ file.sheet1.update(values=[["54"]], range_name="B2")

@NickCrews
Copy link
Contributor Author

But I don't think besides that I would bother changing the code that much. If I had started updating a few days later I would have landed on 6.0.0 and I don't think I would be seeing any of this, so I just got unlucky.

@alifeee
Copy link
Collaborator

alifeee commented Dec 4, 2023

Yes, agreed. With #1362 I have changed the readme to the following

"""

Change Worksheet.update arguments

The first two arguments (values & range_name) have swapped (to range_name & values). Either swap them (works in v6 only), or use named arguments (works in v5 & v6).

As well, values can no longer be a list, and must be a 2D array.

- file.sheet1.update([["new", "values"]])
+ file.sheet1.update([["new", "values"]]) # unchanged

- file.sheet1.update("B2:C2", [["54", "55"]])
+ file.sheet1.update([["54", "55"]], "B2:C2")
# or
+ file.sheet1.update(range_name="B2:C2", values=[["54", "55"]])

"""

@alifeee
Copy link
Collaborator

alifeee commented Dec 4, 2023

Ah, sorry I assumed master was where the 6.0.0 release was getting cut from. I didn't see the feature branch. Probably you have good reason, but if master was tip of tree where 6.0.0 was going to get released from, then I wouldn't be confused. So that is another change you could do

Reasonable, yes. We started the v6.0.0 release a long time ago, and have made many v5 releases since then, so it has had to be on a different branch.

@alifeee
Copy link
Collaborator

alifeee commented Dec 4, 2023

@NickCrews thanks for your input! :)

update is probably the biggest breaking API change of v6.0.0 so it is good to get opinions about it :)

@NickCrews
Copy link
Contributor Author

NickCrews commented Dec 4, 2023

We started the v6.0.0 release a long time ago, and have made many v5 releases since then, so it has had to be on a different branch.

I see make sense. I was expecting the model of branching I was familiar with: you cut off the v5.x.x branch off from master when you do the 5.0 release. Then, ALL new features/commits go to master, and you selectively backport the non-6.0 commits to the v5.x.x branch. But whatever works I don't think one is inherently superior that's just what I was expecting :)

@alifeee
Copy link
Collaborator

alifeee commented Dec 4, 2023

I see make sense. I was expecting the model of branching I was familiar with: you cut off the v5.x.x branch off from master when you do the 5.0 release. Then, ALL new features/commits go to master, and you selectively backport the non-6.0 commits to the v5.x.x branch. But whatever works I don't think one is inherently superior that's just what I was expecting :)

That's how it was layed out when I joined :). Your thoughts interest me though. Do you mean something that looks like this? Where master is always the next major release? What happens if someone makes a fix, they make it on release/5.12.2 and then we tag the 5.12.2 release, and cherry-pick the fix into master? Or do they make the fix on master and we cherry pick it into 5.12.2? Or make it on 5.12.2 and then merge 5.12.2 into master after tagging?

@lavigne958
Copy link
Collaborator

I see make sense. I was expecting the model of branching I was familiar with: you cut off the v5.x.x branch off from master when you do the 5.0 release. Then, ALL new features/commits go to master, and you selectively backport the non-6.0 commits to the v5.x.x branch. But whatever works I don't think one is inherently superior that's just what I was expecting :)

That's how it was layed out when I joined :). Your thoughts interest me though. Do you mean something that looks like this? Where master is always the next major release? What happens if someone makes a fix, they make it on release/5.12.2 and then we tag the 5.12.2 release, and cherry-pick the fix into master? Or do they make the fix on master and we cherry pick it into 5.12.2? Or make it on 5.12.2 and then merge 5.12.2 into master after tagging?

That's a very good point, and I thought about it recently with the comming 6.0.0 release.

I wish to change our branching system to what is described here in the link above.
that is much easier to manage.

What happens if someone makes a fix, they make it on release/5.12.2 and then we tag the 5.12.2 release, and cherry-pick the fix into master?

yes it is, that is one way to go and it works fine, only problem is: you make 2 PRs so one can pass on release/5.12.2 then you make the PR on master and it may fail so that brings you to update again the 5.12.2 branch, but that's fine we can handle it.

Or make it on 5.12.2 and then merge 5.12.2 into master after tagging?

you could but that requires some bot to do what we call the waterfall that when you make a PR it creates for you a second PR toward master and both must pass so the bot allows you to merge. that brings you more complexity with conflict though...

I would go for the first one I think, we should discuss it and make proposals and ballance pros/cons I think.

@NickCrews
Copy link
Contributor Author

Yeah that is what I mean. We always targeted master by default, then after that got merged we would backport to the release branches. I think that puts a little more priority on master over the features, which I think is more intuitive, but the end result should be the same?

Even simpler, and avoids merge conflicts/forgetting to backport, but not always worth it, is to avoid branches altogether and keep things as runtime branches. Two different implementations present at the same time, but abstracted over so the transition is painless:

# app.py
from .my_types import Self
# my_types.py
try:
    # python 3.11+
    from typing import Self as Self
except ImportError:
    # python <3.11
    from typing_extensions import Self as Self

This is another argument for very rapid releases: If you release a major version every month instead of every 6, then there aren't as many concurrent incompatible things you need to support at the same time, and the above becomes much more tenable, but users still have the granularity to pick up only a few breaking changes/features at a time.

@alifeee
Copy link
Collaborator

alifeee commented Dec 4, 2023

what does a user do if they want to make a PR to fix, say 5.12.3, but the 5.12.3 branch doesn't exist yet?

I think all commits should be done to master and if they are required (e.g., bugfix) for a previous version they can be backported

anyway I am interested in new branching system :)

@NickCrews
Copy link
Contributor Author

I think that would be you target the PR to the releases/5.12.Z branch, (the tag v5.12.2 probably was already pointing at this branch), then once that is merged and everything looks good, then you make a new tag v5.12.3. Am I missing something or does that sound right?

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

No branches or pull requests

3 participants