generalimport icon indicating copy to clipboard operation
generalimport copied to clipboard

Allow subclassing from missing dependency

Open ZanSara opened this issue 2 years ago • 5 comments

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?

ZanSara avatar Apr 18 '23 16:04 ZanSara

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 😄

Mandera avatar Apr 19 '23 01:04 Mandera

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:

ZanSara avatar Apr 19 '23 09:04 ZanSara

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

Mandera avatar May 16 '23 02:05 Mandera

Is there any update on this pull request? This generalimport library is super useful, but I really need this subclassing functionality

fjxmlzn avatar Jan 09 '25 07:01 fjxmlzn

Hey there, I'm glad you find it useful! I'm afraid I'm not prioritizing this library as of now, but I'm happy to accept contributions if they fulfill the criteria discussed above 🙏

Mandera avatar Jan 09 '25 11:01 Mandera