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

Use abc.ABC as baseclass for AnyPath to fix issubclass behavior #251

Merged

Conversation

chbehrens
Copy link
Contributor

Following @jayqi's suggestion in #247, this PR lets AnyPath inherit from abc.ABC and sets its subclasses via ABC.register. It also adds a test to check the bug filed in #246.

Fixes #246

@chbehrens chbehrens force-pushed the anypath-subclasses-via-abc branch from cda41c5 to b9036df Compare August 17, 2022 11:53
@pjbull
Copy link
Member

pjbull commented Aug 17, 2022

Hm @chbehrens, looks like mypy and ABC have some rough edges. See python/mypy#2922, python/mypy#5374, https://stackoverflow.com/questions/69330256/how-to-get-an-abstract-dataclass-to-pass-mypy

Ironically, it looks like the metaclass implementation might pass mypy (see python/mypy#1459 (comment))

I'm ok with a # type: ignore on the line that is erroring. That work for you @jayqi?

@jayqi
Copy link
Member

jayqi commented Aug 17, 2022

I'm also fine with a # type: ignore.

I was also looking at python/mypy#5374 and it seems like it's possible this is addressed very recently in mypy (just 5 days ago) by python/mypy#13398. That PR was merged but is not released yet, so I think we'll potentially look at revisiting once the next release of mypy is published.

@chbehrens
Copy link
Contributor Author

Ok, I added the # type: ignore and will try to keep it in mind.

@pjbull
Copy link
Member

pjbull commented Aug 18, 2022

Thanks @chbehrens! You can ignore the failure for now, Github Actions is having an outage:
https://www.githubstatus.com/

I'll rerun these later once they stabilize the service

@pjbull
Copy link
Member

pjbull commented Aug 18, 2022

Changes look good, thanks @chbehrens! I'm going to move this to a local branch to run the live server tests.

@pjbull pjbull changed the base branch from master to 251-live-tests August 18, 2022 18:07
@pjbull pjbull merged commit b5cdc3f into drivendataorg:251-live-tests Aug 18, 2022
pjbull added a commit that referenced this pull request Aug 18, 2022
#257)

* Use abc.ABC as baseclass for AnyPath

* add type ignore for ABC.register

Co-authored-by: chbehrens <chbehrens@users.noreply.github.com>
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.

AnyPath is not a subclass of AnyPath
3 participants