Skip to content

Add GenericModel to MOVED_IN_V2#6776

Merged
adriangb merged 2 commits intomainfrom
add-genericmodel-to-moved
Jul 22, 2023
Merged

Add GenericModel to MOVED_IN_V2#6776
adriangb merged 2 commits intomainfrom
add-genericmodel-to-moved

Conversation

@adriangb
Copy link
Copy Markdown
Member

@adriangb adriangb commented Jul 20, 2023

#6733 (comment)

Selected Reviewer: @dmontagu

@adriangb
Copy link
Copy Markdown
Member Author

please review cc @Kludex

@cloudflare-workers-and-pages
Copy link
Copy Markdown

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
Comment thread pydantic/_migration.py Outdated
'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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

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

@Kludex
Copy link
Copy Markdown
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