Remove API calls from creationTime/lastUpdateTime

[alifeee]

closes #1253

creationTime and lastUpdateTime were @propertys. But, the first time they were used they called the API

gspread/gspread/spreadsheet.py

Lines 52 to 60 in d0f1380

	@property
	def creationTime(self):
	"""Spreadsheet Creation time."""
	try:
	return self._properties["createdTime"]
	except KeyError:
	metadata = self.client._get_file_drive_metadata(self.id)
	self._properties.update(metadata)
	return self._properties["createdTime"]

This is because they are both Drive metadata, and not Sheets metadata. Thus, the drive metadata was fetched the first time the @propertys were accessed.

This PR changes the behaviour of this. In my opinion, there are several options to fix the problem of "Avoid doing i/o in properties" (issue #1253):

1. Remove @property, i.e., change them to methods
   I don't particularly like this since the API call is only made one time, the first time. After that, they act as properties
2. Return None if they do not exist, and expect the user to use refresh_lastUpdateTime or get_lastUpdateTime
   This is not very user-friendly, imo.
3. Throw an Exception if they do not exist, and tell the user to use refresh_lastUpdateTime or get_lastUpdateTime
   Again, not very user friendly
4. Fetch the properties in Spreadsheet.__init__
   This means they can be @propertys just like the other ones (id, title, etc.). However, it means another API call when opening a spreadsheet

I went with option 4 for this PR. Please convince me otherwise if you think another idea is better, or have a different idea.

This solution introduces another API call, but otherwise does not change the public API, as we do not have to deprecate anything*.

• It turns 1 API calls on spreadsheet initialisation to 2
  If using the properties anyway, it would be 2 as soon as you try to access the properties, or 3 if using both.
• I am not sure how auth works properly, but it could introduce problems if a user/key is allowed to access the Sheets API but not the Drive API (!!!)

*: however, I do suggest deprecating lastUpdateTime in favour of get_lastUpdateTime() (which calls the API), since this is something that a user probably always wants the latest version of

todo:

• deprecate lastUpdateTime (add warning/another issue)
• fetch_sheet_metadata should fetch drive api too???

[alifeee]

tests currently broken because of new API request in Spreadsheet.__init__. cassettes need refreshing

[alifeee]

@lavigne958 Before I continue with this I'd like your thoughts on the above, no rush :)

[lavigne958]

that's a nice catch, a single API call at init is ok as it is for a good reason.

will be able to remove the property later too nicely.

[alifeee]

Moving this to here instead of in a review comment...?

Then, yes... we could merge it into v5... if you are happy to introduce an extra API request on Spreadsheet init :)

[alifeee]

Two questions for @lavigne958:

should I add a fetch of the Drive metadata to fetch_sheet_metadata?

gspread/gspread/spreadsheet.py

Lines 239 to 247 in ba16232

	def fetch_sheet_metadata(self, params=None):
	if params is None:
	params = {"includeGridData": "false"}
	url = SPREADSHEET_URL % self.id
	r = self.client.request("get", url, params=params)
	return r.json()

should I turn this warning into a deprecation warning?

gspread/gspread/spreadsheet.py

Lines 62 to 73 in ba16232

	@property
	def lastUpdateTime(self):
	"""Spreadsheet last updated time.
	Only updated on initialisation.
	For actual last updated time, use get_lastUpdateTime()."""
	warnings.warn(
	"""
	This is only updated on initialisation and is probably outdated by the time you use it.
	For an up to date last updated time, use get_lastUpdateTime().
	"""
	)
	return self._properties["modifiedTime"]

[lavigne958]

Two questions for @lavigne958:

should I add a fetch of the Drive metadata to fetch_sheet_metadata?

gspread/gspread/spreadsheet.py

Lines 239 to 247 in ba16232

	def fetch_sheet_metadata(self, params=None):
	if params is None:
	params = {"includeGridData": "false"}
	url = SPREADSHEET_URL % self.id
	r = self.client.request("get", url, params=params)
	return r.json()

I would go for: keep it next to it, it's ok to have both method next ot each others. we only call them in the init method.

should I turn this warning into a deprecation warning?

gspread/gspread/spreadsheet.py

Lines 62 to 73 in ba16232

	@property
	def lastUpdateTime(self):
	"""Spreadsheet last updated time.
	Only updated on initialisation.
	For actual last updated time, use get_lastUpdateTime()."""
	warnings.warn(
	"""
	This is only updated on initialisation and is probably outdated by the time you use it.
	For an up to date last updated time, use get_lastUpdateTime().
	"""
	)
	return self._properties["modifiedTime"]

I would say YES, but I feel like we are removing so much code 🙈

[alifeee]

I would go for: keep it next to it, it's ok to have both method next ot each others. we only call them in the init method.

I do not understand this. The options are:

• fetch drive metadata in spreadsheet init
• fetch drive metadata in spreadsheet.fetch_sheet_metadata

I do the first. I will stick with that for now, because to do otherwise would mean refreshing the cassettes again...

I would say YES, but I feel like we are removing so much code 🙈

This is fair. Perhaps we just leave in a warning

[lavigne958]

I would go for: keep it next to it, it's ok to have both method next ot each others. we only call them in the init method.

I do not understand this. The options are:

• fetch drive metadata in spreadsheet init
• fetch drive metadata in spreadsheet.fetch_sheet_metadata

I do the first. I will stick with that for now, because to do otherwise would mean refreshing the cassettes again...

Agreed, to me it looks good like this. the first option you suggest is fine.

I would say YES, but I feel like we are removing so much code see_no_evil

This is fair. Perhaps we just leave in a warning

yep that's fine like that, it does not break a thing, people can use the new function, sounds nice for everyone.

[alifeee]

okay, super

Changes are:

• rename Client._get_file_drive_metadata to Client.get_file_drive_metadata (change to public method, as it is required to be used in Spreadsheet.__init__().
• Fetch Drive metadata on Spreadsheet.__init__(). This adds an API call, but means that creationTime and lastUpdateTime properties of Spreadsheet are now static methods (this is the original desire from the issue Avoid doing i/o in properties #1253)
• make Spreadsheet.creationTime and Spreadsheet.lastUpdateTime static properties
• add warning for Spreadsheet.lastUpdateTime. Users should instead use Spreadsheet.get_lastUpdateTime()
• add Spreadsheet.get_lastUpdateTime()
• add tests

[lavigne958]

12k lines of diff, nothing to see here, please keep going 😂

Great job l'été get this merge. So it start adding content for next release. 😁