bellingcat / auto-archiver

Automatically archive links to videos, images, and social media content from Google Sheets (and more).
https://pypi.org/project/auto-archiver/
MIT License
578 stars 60 forks source link

gspread 6.0.0 - be careful #124

Closed djhmateer closed 9 months ago

djhmateer commented 9 months ago

I'm pinning gspread for the time being to 5.12.4 as I found an issue in 6.0.0 version:

https://github.com/bellingcat/auto-archiver/blob/main/src/auto_archiver/feeders/gsheet_feeder.py#L60

  # 30th Jan - refresh the status just in case
  status = gw.get_cell(row, 'status', fresh=original_status in ['', None])

  # 30th Jan - when refreshed cell comes back, it is now a string 'None'
  # I had just done a pipenv update to gspread 6.0.0
  # reverting to 5.12.4 works as expected
  if status not in ['', None]: continue

  # this worked with 6.0.0 but not happy as it may have other effects in codebase
  # if status not in ['', None, 'None']: continue
GalenReich commented 9 months ago

Thanks @djhmateer, pinning to 5.12.4 seems like a sensible idea until we know if there are any cases that are broken by gspread 6.0.0.

Looks like this change occurred here: https://github.com/burnash/gspread/commit/6457676cdf9897b3b53827124f1e1265e9ae5e20

msramalho commented 9 months ago

Pinning is a good decision for now.

Adapting to the new version should not be too hard and require changes only to https://github.com/bellingcat/auto-archiver/blob/main/src/auto_archiver/utils/gworksheet.py

namely https://github.com/bellingcat/auto-archiver/blob/590d3fe824a57b520396d27f59d4a90f275d06ce/src/auto_archiver/utils/gworksheet.py#L67-L68 should be

        if fresh:
            val = self.wks.cell(row, col_index + 1).value
            return val if val != "None" else ""

to keep consistency.

I'm not making the changes yet since testing if there were other changes to gspread.worksheet.get_values() to return "None"s too. But those are the only 2 places we interact with the gspread.worksheet to read the values.

msramalho commented 9 months ago

gspread devs were quick to acknowledge this and there will be a patch in 6.0.1: https://github.com/burnash/gspread/issues/1403

msramalho commented 9 months ago

warning stands: do not use gspread 6.0.0, we can fix it at >=6.0.1 after that's released and tested.