Fix warning message for worksheet.update method

[ksj20]

closes #1310

Fixed the warning message for the worksheet update method for better readability

[alifeee]

Thanks for this change! :)

The warning is clearer now.

Now that you understand why the warning is in place, I wonder if you have any opinion on this:

I get confused, does this warning appear for every .update method? Not even when I change the type of values into a list of list?

Can you think of a way of validating that a user has used kwargs, and not args? I can't. If there is no way, then this PR is complete and we can merge it now.

[ksj20]

1. add the warning under the is_scalar if branch

   gspread/gspread/worksheet.py

   Line 1195 in d076c70

   if is_scalar(range_name):

2. I guess you are looking for only allowing kwargs, something like the below? Then probably catch the TypeError? This is Python 3.10.4 by the way

>>> def func(*, a=None, b=None):
...     return a, b
...
>>> func(1,2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: func() takes 0 positional arguments but 2 were given
>>> func(a=1,b=2)
(1, 2)

[alifeee]

It was my understanding that the primary purpose of the warning was not to ensure that values was a 2D array, but to warn of the swapping of values and range in the next version (6.0.0)

Thus, I was thinking:

"Is there a way of telling if the user called update("A1:A2", [[1, 2]]) or update(range_name="A1:A2", values=[[1, 2]])?"

If there is:

• we can make the warning only happen for the former

If there isn't:

• we have to send the warning even if the user is doing the "correct" behaviour

[ksj20]

Yes, basically my 2nd point solves that issue. In this case, it can be defined as below:

>>> def update(*, range_name=None, values=[[]]):
...     return range_name, values
...
>>> update("A1:A2", [[1, 2]])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: update() takes 0 positional arguments but 2 were given
>>> update(range_name="A1:A2", values=[[1, 2]])
('A1:A2', [[1, 2]])

Take a look at the error message where 0 positional arguments ( arguments in update("A1:A2", [[1, 2]]) ) are allowed and only keyword arguments (named arguments) are allowed.
Note that there is an asterisk(*) before the keyword arguments are defined.
For reference, see this SO answer and Python 3 documentation

[alifeee]

That is good to know.

However, I would not like to disallow using arguments. It should always be allowed in version 5, otherwise it will break people's code. I would like to be able to see if someone has used named arguments, while still allowing the use of normal arguments.

[ksj20]

Yeah, look at this StackOverflow question, Can I distinguish positional and keyword arguments from inside the called function?

According to the SO question, to achieve what you said it should be something like this:

def update(*args, range_name=None, values=[[]]):
    if args: # args is not empty - user passed deprecated positional arguments
        print((
            "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=) "
        ))
        range_name = args[0]
        if len(args) >= 2:
            values = args[1]
    return range_name, values

update('A5', [[1,2,3]]) # Warning appears
update(range_name=None, values=[[1,2,3]]) # No warning

For version 5, you should be doing it in the above way, but for version 6 I think you should be using asterisks before named arguments to make the function more strict

[alifeee]

Ah, I missed that when I read the thread! Good investigating :)

According to the SO question, to achieve what you said it should be something like this:
...

That looks good :). Would you like to implement it with this PR?

[...] for version 6 I think you should be using asterisks before named arguments to make the function more strict

I disagree, I think it is fine to leave it callable using arguments. I will also ask @lavigne958's opinion, since this is v6 related.

[lavigne958]

Hi I think this is a bit overkilled, the version 6 is still not ready but at some point it will be and we don't want to wait too much either.

To le we should keep the warning and as soon as we can we release version 6.

In this way, if the user has updated the parameters I would recommend using some env variables to disable the warning. If the user sets the env variable we don't print the warning.

As simple as that. And it's the users choice to disable that warning message.

[alifeee]

hey, thanks again for this. will merge