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

Defer Django and sqlalchemy imports #1071

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

Conversation

adamchainz
Copy link

Fixes #1070.

@adamchainz adamchainz force-pushed the defer_sqlalchemy_imports branch from a920584 to ee8d278 Compare March 29, 2024 11:25
Comment on lines -65 to -72
try:
from . import mogo
except ImportError:
pass
try:
from . import mongoengine
except ImportError:
pass
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two submodules don’t have any library-specific imports, so the try/except was never needed.

Copy link
Member

@francoisfreitag francoisfreitag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi!

Thanks for offering a PR! 🌟

I think these imports are here to allow code such as import factory; factory.mogo.MogoFactory().

Conversely, users who have django as a dependency but do not use it would probably rather avoid loading django at all.

I have suggested such a simplification in the past #760 (see the discussion there) and follow up #766. It was rejected on the basis that it would break imports for users.

Arguably, this code pattern should not have been supported in the first place. I’m still of the opinion this change should land (maybe with a deprecation period to reduce friction for users).

This PR adds another good argument: performance.

@francoisfreitag francoisfreitag requested a review from rbarrois April 4, 2024 16:30
@francoisfreitag
Copy link
Member

Oopsie, I now realize I missed line 5 : from . import alchemy, mogo, mongoengine. Edited my previous comment.

Your suggested change is much less controversial then. :)

@francoisfreitag
Copy link
Member

I’m wondering if the same change should be applied to the django module?

@adamchainz adamchainz force-pushed the defer_sqlalchemy_imports branch from ee8d278 to 6fc6f82 Compare April 9, 2024 13:14
@adamchainz adamchainz changed the title Defer sqlalchemy import Defer Django and sqlalchemy imports Apr 9, 2024
@adamchainz adamchainz force-pushed the defer_sqlalchemy_imports branch from 6fc6f82 to 79bde24 Compare April 9, 2024 13:15
@adamchainz
Copy link
Author

Yeah, good idea. I didn’t do it at first out of laziness. I just made the change and it wasn’t that hard!

@rbarrois
Copy link
Member

rbarrois commented Apr 9, 2024

I understand the idea behind this change; however, I would prefer to have an agreement on the overall design (in the issue) before jumping into code.

At a high level, I would be OK with this kind of change if:

  • It doesn't require any change on current working code;
  • It doesn't scatter import statements throughout the codebase;
  • It breaks cleanly at module import time, not at runtime — i.e as soon as one declares a DjangoModelFactory / AlchemyModelFactory / etc, not one they get used.

Would it be possible to adjust this idea to match those? I'd rather avoid increasing the burden on maintenance for the library — including issues opened by users who don't understand why they get import errors when calling their factories…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Defer sqlalchemy import
3 participants