Rename EntityExtractorMixin #11356
Rename EntityExtractorMixin #11356
Conversation
Co-authored-by: AlGouvea <[email protected]>
Co-authored-by: AlGouvea <[email protected]>
Co-authored-by: AlGouvea <[email protected]>
VitorLamego
force-pushed
the
rename-EntityExtractorMixin
branch
from
503be26
to
ce0c7db
Compare
VitorLamego
changed the title
There was a problem hiding this comment.
Thanks for addressing this!
The migration guide in the docs should also be updated to indicate the change. Since the migration will differ for users before and after the rename, I'd suggest adding a callout like e.g.
:::info Changed in 3.3
As of 3.3, the `EntityExtractorMixin` has been renamed back to `EntityExtractor`. The migration instructions below are valid for versions >= 3.0, <3.3.
:::
|
@VitorLamego I'm adding one thing, which is a backwards compatibility |
|
Since I can't push to your fork, there's a branch & commit here with the suggested changes - could you pull these into your branch? |
indam23
dismissed
their stale review
Will re-approve once deprecated class is added back for backwards compatibility
|
Done! |
|
Thanks, do you mind addressing the failing CI checks ? They're my fault, I added the logging module when testing locally. |
|
No problem! Is there any way I could help on documentation check ?? @melindaloubser1 |
…a into rename-EntityExtractorMixin
|
Done @melindaloubser1 ! |
|
Everything ok ? @melindaloubser1 |
There was a problem hiding this comment.
@VitorLamego Approving this as Melinda is no longer a maintainer for this repo.
Please run make formatter to resolve the formatting check fail. Before that ensure that the version of black in your venv is 22.6.0.
Also you can run make lint and make types to check for any additional errors that this Code quality check could uncover and fix them before committing.
|
|
VitorLamego
force-pushed
the
rename-EntityExtractorMixin
branch
from
b964432
to
eb19a38
Compare
|
Sorry for the delay! I believe it is ok now :) @ancalita |
Co-authored-by: VitorLamego <[email protected]>
Co-authored-by: VitorLamego <[email protected]>
|
It seems that errors on CI are not due to this issue, right ?? Anything else we could do to help ? @ancalita |
VitorLamego
requested review from
indam23 and
ancalita
and removed request for
ancalita and
indam23
| This class will be removed in 4.0 | ||
| """ | ||
|
|
||
| def __init_subclass__(cls, **kwargs: Any): |
There was a problem hiding this comment.
You also need to specify type hint for cls.
I've restarted the jobs, could have been flaky incidents. |
| This class will be removed in 4.0 | ||
| """ | ||
|
|
||
| def __init_subclass__(cls, **kwargs: Any): |
There was a problem hiding this comment.
| def __init_subclass__(cls, **kwargs: Any): | |
| def __init_subclass__(cls: EntityExtractor, **kwargs: Any): |
Co-authore-by: AlGouvea [email protected]
Proposed changes:
EntityExtractorMixinfor consistency #10225 references of EntityExtractorMixin were renamed to EntityExtractor for consistency.Status (please check what you already did):
black(please check Readme for instructions)