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

Handle subscripted generics in query selector #3980

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

davetapley
Copy link
Contributor

Same as #3805 but change my branch off main

Fixes:

TypeError: Subscripted generics cannot be used with class and instance checks

But requires Python 3.9:
https://peps.python.org/pep-0585/

Textualize#2867
@rodrigogiraoserrao
Copy link
Contributor

Hey Dave, thanks for the PR.
Would you mind adding a couple of regression tests? 🙌

@davetapley
Copy link
Contributor Author

@rodrigogiraoserrao I can but it won't be pretty because:

The fix requires Python 3.9 (for https://peps.python.org/pep-0585/), but the tests also run on 3.8.

Note: #2867 is a hard crash anyway, so fix is no worse than existing behavior on 3.8.

For tests can either:

  1. Defer until 3.8 support is dropped (easiest)
  2. Only run test on >= 3.9
  3. Expect exception on 3.8, work on >= 3.9

Any preference?

@rodrigogiraoserrao
Copy link
Contributor

Hey Dave,

Maybe I misunderstood what you're saying but are you suggesting that adding a couple of regression tests and marking them with a @skipif that checks if we're on Python 3.9+ is ugly?

I agree that the new change is no worse than the current behaviour and thus I'm personally a fan of this change but we typically don't add new features without tests!

@davetapley
Copy link
Contributor Author

@rodrigogiraoserrao I was, but I didn't know about skipif, that makes it look quite easy 😁
I'll get some tests in ASAP.

@willmcgugan
Copy link
Collaborator

Would I be correct in saying this wouldn't check the generic type, just the class?

i.e. the following would return all DataTable, not just float Datatables?

self.query("*", "DataTable[float]")

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.

None yet

3 participants