Fix missing attribute spreadsheet in Worksheet.

[lavigne958]

The Worksheet object was missing the spreadsheet attribute.

Add it back to be backward compatible.

closes #1395

Signed-off-by: Alexandre Lavigne lavigne958@gmail.com

[alifeee]

looks good.

it is worrying that we do not have to change any tests for this behaviour.

Perhaps we should add one?

[lavigne958]

looks good.

it is worrying that we do not have to change any tests for this behaviour.

Perhaps we should add one?

you're right, I added a safeGuard test to keep access to public attributes and check their types too.

[alifeee]

good job with the recursive type-checking! :)

looking at this made me think of something. In v5 -> v6, the Worksheet constructor is changed

class Worksheet
  def __init__(
    self,
-   spreadsheet,
+   spreadsheet_id: str,
+   client: HTTPClient,
    properties
  ):

This is a breaking change. It should be documented in the migration guide, as people now have to construct Worksheets differently.

Alternatively, this PR reintroduces the spreadsheet argument to Worksheet.__init__(), so we could try to make the __init__ signature the same as in v5, so it is not a breaking change.

This would probably look something like

class Worksheet
  def __init__(
    self,
    spreadsheet,
    properties,
+   client: HTTPClient = None,
  ):
+   self.spreadsheet_id = spreadsheet.id
+   if client is None:
+      # raise warning/throw error/make client HTTPClient

Then people could still use gspread.Worksheet(spreadsheet, properties) as in v5.

What do you think?

[lavigne958]

good job with the recursive type-checking! :)

Thanks 🙏

looking at this made me think of something. In v5 -> v6, the Worksheet constructor is changed

class Worksheet
  def __init__(
    self,
-   spreadsheet,
+   spreadsheet_id: str,
+   client: HTTPClient,
    properties
  ):

This is a breaking change. It should be documented in the migration guide, as people now have to construct Worksheets differently.

Alternatively, this PR reintroduces the spreadsheet argument to Worksheet.__init__(), so we could try to make the __init__ signature the same as in v5, so it is not a breaking change.

This would probably look something like

class Worksheet
  def __init__(
    self,
    spreadsheet,
    properties,
+   client: HTTPClient = None,
  ):
+   self.spreadsheet_id = spreadsheet.id
+   if client is None:
+      # raise warning/throw error/make client HTTPClient

Then people could still use gspread.Worksheet(spreadsheet, properties) as in v5.

What do you think?

This should not be a problem as the only way to obtain a worksheet is through the spreadsheet object (using like get_worksheet()).
So this means users are not expected to instantiate the object themselves. So any changes here should be transparent and not a breaking change.

That's how the project is designed at least, but you never know what users could do with it.

Then we must raise an error like you propose if someone tried to instantiate it with no client, so even if we keep the same constructor signature their code will raise some error anyway so they'll need to passe the HTTP client anyway.

I understand the issue, though to me it's not an expected use case by gspread.

[alifeee]

This should not be a problem as the only way to obtain a worksheet is through the spreadsheet object

This may be the most common way, but the Worksheet object is publicly accessible, no? Any user can use it like,

import Worksheet from gspread.Worksheet
... 
my_custom_worksheet = Worksheet(...)

So changing the initialisor signature would break this.

So this means users are not expected to instantiate the object themselves

you can never expect users to only do as expected ;)

So maybe we make the breaking change still, but put it in the migration guide?

[lavigne958]

This should not be a problem as the only way to obtain a worksheet is through the spreadsheet object

This may be the most common way, but the Worksheet object is publicly accessible, no? Any user can use it like,

import Worksheet from gspread.Worksheet
... 
my_custom_worksheet = Worksheet(...)

So changing the initialisor signature would break this.

So this means users are not expected to instantiate the object themselves

you can never expect users to only do as expected ;)

So maybe we make the breaking change still, but put it in the migration guide?

You're right, I will upgrade it so the signature matches + the new extra argument (the HTTP Client)

We need to raise in case the HTTP Client is not given at least users will have an explicit error message here.

Can you add it to the migraine migration guide please ?
(spellchecker.... 🤦 I hope I don't give YOU a migraine 😆 )

[alifeee]

this looks good I will write the migration guide very soon :)