Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix type issue (remove .first() function) #1344

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

alifeee
Copy link
Collaborator

@alifeee alifeee commented Nov 5, 2023

this was not caught by #1296's CI because type-checking was not yet enabled in the workflow.

This is the main change:

- value = str(data.first())
+ try:
+     value = str(data[0][0])
+ except IndexError:
+     value = str(None)

...which replaces the call to ValueRange.first() with its implementation

def first(self, default: Optional[str] = None) -> Optional[str]:
"""Returns the value of a first cell in a range.
If the range is empty, return the default value.
"""
try:
return self[0][0]
except IndexError:
return default

The error was because before #1296, get returned a ValueRange. After #1296, it returns ValueRange | List[List[Any]], so the type checking complained that .first() was not a method on List[List[Any]]. I'm not sure how to solve this issue by typinig, which is why I did what I did here.

Why is .first() needed as a function anyway? It is hardly used. I propose that we don't need it (but we should keep it, and just not use it internally).

@alifeee alifeee added this to the 6.0.0 milestone Nov 5, 2023
@alifeee alifeee requested a review from lavigne958 November 5, 2023 17:42
@alifeee alifeee self-assigned this Nov 5, 2023
@alifeee alifeee merged commit 735feb8 into feature/release_6_0_0 Nov 6, 2023
10 checks passed
@alifeee alifeee deleted the fix/type-issue branch November 6, 2023 22:36
@alifeee alifeee mentioned this pull request Jan 26, 2024
@msramalho
Copy link

FYI the changes made the default go from None to "None" which is, to the best of my knowledge, not described in the migration to 6.0.0. Are there more such breaking changes related to dropping first() or how the data is read and returned in general?

eg impact: bellingcat/auto-archiver#124

@alifeee
Copy link
Collaborator Author

alifeee commented Feb 1, 2024

FYI the changes made the default go from None to "None" which is, to the best of my knowledge, not described in the migration to 6.0.0. Are there more such breaking changes related to dropping first() or how the data is read and returned in general?

This is simply a mistake on our part. There is no reason to change the default behaviour, a change that we didn't see with this PR.

I'll change it back to the previous argument and make a minor release.

@msramalho
Copy link

Oh that's brilliant, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants