search

ManderaGeneral / generalimport

Handle all your optional dependencies with a single call!
https://pypi.org/project/generalimport/
Apache License 2.0
16 stars 1 forks source link

Allow subclassing from missing dependency #28

Open ZanSara opened 1 year ago

ZanSara commented 1 year ago

Fixes https://github.com/ManderaGeneral/generalimport/issues/27

This PR adds an implementation of __mro_entries__ to FakeModule. This method returns a "FakeBaseClass" class whose __init__ (and __new__, for good measure) trigger a MissingOptionalDependency exception. In this way, subclasses from missing dependencies can be created, but will fail as soon as they're initialized. See the unit test.

Note: overriding __init__ might not be necessary but I don't see downsides of doing so right now. Let me know if you prefer to stick to __new__ only.

:warning: The unit tests I wrote gets automatically skipped. Do you have any idea why is that?

Mandera commented 1 year ago

I think this has potential! However I would like subclassing `FakeModule to recursively return itself, just like getting an attr of it does!

I tried a little bit to make that work but I couldn't. I added a test that I'd like to pass for example (Just one of the many use cases)

I added two commits! This is also an experiment on my part, I've never added commits to someone else's PR before. I'm afraid it could be frowned upon. Should I perhaps have credited you in the commit message or not committed at all? I have no idea what the general consensus is, please let me know 😄

ZanSara commented 1 year ago

I would like subclassing `FakeModule to recursively return itself, just like getting an attr of it does!

Same, I wanted to do that too at the start, but I couldn't make it work either :sweat_smile: Then I tried to use a plain object, but that also didn't work (MRO resolution errors or any ABC class). That's why I decided to use that dynamically generated class. There might be other ways too!

Don't forget that even if we want to use FakeModule for it, __new__ and __init__ need to be altered to throw the exception, so I'm not sure how viable it is really. Probably not worth the effort.

However, one thing we can do it to make FakeBaseClass a "real" class instead of generating it dynamically. I'm gonna add a commit that does this, so we can see how it looks like.

I added a test that I'd like to pass for example (Just one of the many use cases)

Thanks! I'll try to make that work too :+1:

I added two commits! This is also an experiment on my part, I've never added commits to someone else's PR before. I'm afraid it could be frowned upon. Should I perhaps have credited you in the commit message or not committed at all? I have no idea what the general consensus is, please let me know

I don't mind, no problem! The only issue I see is that if I was working on the same files and you committed in the meantime, it could become a mess to merge. So normally when I commit on someone's PR I make sure they're not actively working on it and I announce it before doing it, just to avoid that situation.

As long as it's just us two I don't really mind though :smile:

Mandera commented 1 year ago

Dev workflow is functional! The test I added is failing here. I am leaving this for now, taking a closer look later!