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

create_model support generics model #3946

Merged
merged 2 commits into from Aug 11, 2022
Merged

Conversation

hot123s
Copy link
Contributor

@hot123s hot123s commented Mar 28, 2022

Change Summary

create_model support generics model

Related issue number

fix #3945

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

@hot123s
Copy link
Contributor Author

hot123s commented Mar 28, 2022

please review

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, but I wonder if this could have any unintended side effects?

@PrettyWood @hramezani?

@samuelcolvin samuelcolvin mentioned this pull request Aug 8, 2022
14 tasks
@hramezani
Copy link
Member

but I wonder if this could have any unintended side effects?

I am not sure. but we need a rebase here to properly handle __cls_kwargs__ and kwds

please update

@hot123s
Copy link
Contributor Author

hot123s commented Aug 9, 2022

I try to extract code from types.new_class to avoid using lambda function

this is source code in python3.8 official library

def new_class(name, bases=(), kwds=None, exec_body=None):
    resolved_bases = resolve_bases(bases)
    meta, ns, kwds = prepare_class(name, resolved_bases, kwds)
    if exec_body is not None:
        exec_body(ns)
    if resolved_bases is not bases:
        ns['__orig_bases__'] = bases
    return meta(name, resolved_bases, ns, **kwds)

I'm alse not sure types.new_class could have any unintended side effects in this project.But this is a official function to create new class, and maybe this is good alternative for type

@samuelcolvin @hramezani

@hramezani
Copy link
Member

hramezani commented Aug 9, 2022

Thanks @hot123s for the explanations.

Please consider you need to rebase your patch first.

Copy link
Member

@PrettyWood PrettyWood left a comment

Choose a reason for hiding this comment

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

LGTM! IMO it's ok to add this in v1.10

@hot123s hot123s force-pushed the master branch 2 times, most recently from c088109 to d51585c Compare August 11, 2022 06:15
@hot123s
Copy link
Contributor Author

hot123s commented Aug 11, 2022

@hramezani I have rebased my branch, and compatible with __cls_kwargs__, and add corresponding test case.Please review it.

Copy link
Member

@hramezani hramezani left a comment

Choose a reason for hiding this comment

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

Thanks @hot123s 👍

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.

otherwise LGTM.

changes/3945-hot123s.md Outdated Show resolved Hide resolved
@samuelcolvin samuelcolvin enabled auto-merge (squash) August 11, 2022 10:32
@samuelcolvin
Copy link
Member

Thanks so much.

@samuelcolvin
Copy link
Member

See #4432 related workaround which I think is required due to this PR.

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.

create_model not support generics model
4 participants