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

Allow subclassing from missing dependency #28

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

ZanSara
Copy link
Contributor

@ZanSara ZanSara commented Apr 18, 2023

Fixes #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.

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

ZanSara and others added 6 commits April 18, 2023 17:48
Signed-off-by: Mandera <rickard.abraham@gmail.com>
Signed-off-by: Mandera <rickard.abraham@gmail.com>
@Mandera
Copy link
Contributor

Mandera commented Apr 19, 2023

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
Copy link
Contributor Author

ZanSara commented Apr 19, 2023

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 😅 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 👍

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 😄

ZanSara and others added 3 commits April 19, 2023 11:54
# Conflicts:
#	generalimport/fake_module.py
#	generalimport/test/test_generalimport.py
#	generalimport/test/test_usage/test_object.py
Expecting test_subclass_class_returning_self to fail
@Mandera
Copy link
Contributor

Mandera commented May 16, 2023

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

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.

Allow subclassing from missing dependency
2 participants