Skip to content

Conversation

adriangb
Copy link
Member

@adriangb adriangb commented Jul 20, 2023

#6733 (comment)

Selected Reviewer: @dmontagu

@adriangb
Copy link
Member Author

please review cc @Kludex

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jul 20, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 9c63a84
Status: ✅  Deploy successful!
Preview URL: https://8db80fd0.pydantic-docs2.pages.dev
Branch Preview URL: https://add-genericmodel-to-moved.pydantic-docs2.pages.dev

View logs

Kludex
Kludex previously approved these changes Jul 20, 2023
@Kludex Kludex dismissed their stale review July 20, 2023 13:07

wait

Kludex
Kludex previously requested changes Jul 20, 2023
@@ -12,6 +12,7 @@
'pydantic.utils:to_lower_camel': 'pydantic.alias_generators:to_camel',
'pydantic:PyObject': 'pydantic.types:ImportString',
'pydantic.types:PyObject': 'pydantic.types:ImportString',
'pydantic.generics.GenericModel': 'pydantic.BaseModel',
Copy link
Member

Choose a reason for hiding this comment

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

I think GenericModel should either on REMOVED_IN_V2 or it should have a special condition like we have on BaseSettings on the getattr_migration. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what you mean, can you just push your suggestion?

@pydantic-hooky pydantic-hooky bot added awaiting author revision awaiting changes from the PR author and removed ready for review labels Jul 20, 2023
@pydantic-hooky pydantic-hooky bot assigned adriangb and unassigned dmontagu Jul 20, 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.

Not sure what @Kludex is talking about but I'm fine with this, or with whatever other improvement over this @Kludex might have in mind.

@adriangb adriangb enabled auto-merge (squash) July 22, 2023 09:10
@adriangb adriangb merged commit 6ef959f into main Jul 22, 2023
@adriangb adriangb deleted the add-genericmodel-to-moved branch July 22, 2023 09:10
@Kludex
Copy link
Member

Kludex commented Jul 22, 2023

No, it was not... I was going to get to this today, I didn't have time the last days.

Should be fine, but I'm not sure if this is what we are supposed to do. Like, I think it should be on the REMOVE_IN_V2 list instead of MOVE, because it was not moved, it's just removed. 🤔

@adriangb
Copy link
Member Author

But functionally speaking if someone replaces GenericModel with BaseModel they’re good right?

@Kludex
Copy link
Member

Kludex commented Jul 22, 2023

But functionally speaking if someone replaces GenericModel with BaseModel they’re good right?

Correct. 👍

It's just that the warning is "GenericModel has been moved to BaseModel" - or similar. Anyway, it's fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting author revision awaiting changes from the PR author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants