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.

Sign up for GitHub

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

Jump to bottom

Defer Django and sqlalchemy imports #1071

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

Defer Django and sqlalchemy imports #1071

wants to merge 1 commit into from

Conversation

adamchainz
Copy link

Fixes #1070.

adamchainz
adamchainz commented Mar 29, 2024
View reviewed changes
factory/__init__.py
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.

francoisfreitag
francoisfreitag reviewed Apr 4, 2024
View reviewed changes
Copy link
Member

@francoisfreitag francoisfreitag left a comment •
edited
Loading

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
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 changed the title Defer sqlalchemy import Defer Django and sqlalchemy imports Apr 9, 2024
@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
Reviewers

@francoisfreitag francoisfreitag francoisfreitag approved these changes

@rbarrois rbarrois Awaiting requested review from rbarrois

Assignees
No one assigned
Labels
DesignDecision
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Defer sqlalchemy import
3 participants
@adamchainz @francoisfreitag @rbarrois