Closed NickCrews closed 9 months ago
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.
This issue being created tells me that the situation is unclear. We can solve it in a couple of ways
In fact, in #1336 I added code that swaps the arguments if they "seem" the wrong way round:
Thus, another option is:
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)
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
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
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")
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.
Yes, agreed. With #1362 I have changed the readme to the following
"""
Worksheet.update
argumentsThe 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"]])
"""
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.
@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 :)
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 :)
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?
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 onrelease/5.12.2
and then we tag the 5.12.2 release, and cherry-pick the fix intomaster
? Or do they make the fix onmaster
and we cherry pick it into5.12.2
? Or make it on5.12.2
and then merge5.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.
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.
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 :)
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?
I just updated to the newest version and am now getting the DeprecationWarning added in https://github.com/burnash/gspread/issues/1209
Before I was using the API of
worksheet.update(data)
to update every cell. However, if I now tryworksheet.update(range_name=":", values=data)
, I getgspread.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.