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

🔥 Remove _base_class_defined hack in favor of an empty bases check #5761

Merged
merged 2 commits into from
May 17, 2023

Conversation

lig
Copy link
Contributor

@lig lig commented May 15, 2023

Change Summary

Remove _base_class_defined hack and check for empty bases as an indicator for detecting BaseModel in ModelMetaclass. These classes are no longer tied to their places in the module.

Related issue number

None

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @adriangb

@lig lig marked this pull request as ready for review May 15, 2023 17:42
@lig
Copy link
Contributor Author

lig commented May 15, 2023

please review

pydantic/main.py Outdated Show resolved Hide resolved
@dmontagu
Copy link
Contributor

I'll just note this may be in tension with some of the goals of #5740 — simplifying the MRO etc.

I like getting rid of the hacky check for subclass of BaseModel, but I don't have a strong opinion about what's best here (or whether there might be other unintended consequences, especially for performance). Happy to defer to @adriangb or @samuelcolvin

@lig
Copy link
Contributor Author

lig commented May 15, 2023

I'll just note this may be in tension with some of the goals of #5740 — simplifying the MRO etc.

I think, it would be even better. I'm happy to remove _BaseModelBase if ModelBase doesn't have any other bases.

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

I'm okay with this in principle, but better if we can reduce the size of the change.

pydantic/main.py Outdated
# track of whether we've created the `BaseModel` class yet, and therefore whether it's safe to refer to it. If
# it *hasn't* been created, we assume that the `__new__` call we're in the middle of is for the `BaseModel`
# class, since that's defined immediately after the metaclass.
if bases == (_BaseModelBase,):
Copy link
Member

Choose a reason for hiding this comment

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

can we change this to if bases != ... and then revert most of these changes here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've minimized the changeset.

IMO, it would be nice to have

if condition:
    # a couple of lines for a special case
    return
# a lot of lines for the main body
return

instead of

if condition:
    # a lot of lines for the main body
    return
else:
    # a couple of lines for a special case
    return

pydantic/main.py Outdated Show resolved Hide resolved
@lig lig force-pushed the burn-base-class-defined-hack branch from 53eeb5b to 1c5b48f Compare May 16, 2023 15:36
@cloudflare-pages
Copy link

cloudflare-pages bot commented May 16, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8dfc06c
Status: ✅  Deploy successful!
Preview URL: https://72936651.pydantic-docs2.pages.dev
Branch Preview URL: https://burn-base-class-defined-hack.pydantic-docs2.pages.dev

View logs

@lig lig force-pushed the burn-base-class-defined-hack branch from 1c5b48f to f814d6b Compare May 16, 2023 15:42
Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

LGTM. @adriangb and @dmontagu are you happy with this?

@lig lig changed the title 🔥 Remove _base_class_defined hack in favor of a marker base class 🔥 Remove _base_class_defined hack in favor of an empty bases check May 16, 2023
Copy link
Contributor

@dmontagu dmontagu left a comment

Choose a reason for hiding this comment

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

I'm good with the change, I just think the big comment that seems to have been moved verbatim probably deserves an update before merging.

@lig lig force-pushed the burn-base-class-defined-hack branch from f814d6b to afde015 Compare May 17, 2023 14:12
@lig lig force-pushed the burn-base-class-defined-hack branch from afde015 to c109adb Compare May 17, 2023 14:15
pydantic/main.py Outdated Show resolved Hide resolved
Co-authored-by: David Montague <35119617+dmontagu@users.noreply.github.com>
@lig lig merged commit 793b484 into main May 17, 2023
43 checks passed
@lig lig deleted the burn-base-class-defined-hack branch May 17, 2023 14:31
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.

None yet

4 participants