Skip to content

instrument_declarative calls a not existing registry method #6291

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.

Sign up for GitHub

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

Jump to bottom
Closed
CaselIT opened this issue Apr 15, 2021 · 7 comments
Closed

instrument_declarative calls a not existing registry method #6291

CaselIT opened this issue Apr 15, 2021 · 7 comments
Assignees
@CaselIT
Labels
bug Something isn't working declarative has to do with the declarative API, scanning classes and mixins for attributes to be mapped regression something worked and was broken by a change
Milestone
1.4.x

Comments

@CaselIT
Copy link
Member

CaselIT commented Apr 15, 2021

Describe the bug

This is not tested anywhere, but it does not seems to used by anyone, since @bryanforbes noticed it while creating the typing

Expected behavior

sqlalchemy/lib/sqlalchemy/ext/declarative/extensions.py

Lines 29 to 37 in b3d764f

def instrument_declarative(cls, cls_registry, metadata):
"""Given a class, configure the class declaratively,
using the given registry, which can be any dictionary, and
MetaData object.
"""
return registry(
metadata=metadata, class_registry=cls_registry
).instrument_declarative(cls)

@CaselIT CaselIT added requires triage New issue that requires categorization bug Something isn't working declarative has to do with the declarative API, scanning classes and mixins for attributes to be mapped regression something worked and was broken by a change and removed requires triage New issue that requires categorization labels Apr 15, 2021
@zzzeek zzzeek added this to the 1.4.x milestone Apr 15, 2021
@zzzeek
Copy link
Member

zzzeek commented Apr 15, 2021

nobody has noticed

@CaselIT
Copy link
Member Author

CaselIT commented Apr 15, 2021

indeed, I guess we could have just removed it instead of deprecating it.

still since it's there it should probably do something other than raise argument error

@zzzeek
Copy link
Member

zzzeek commented Apr 15, 2021

we should fix it because it hasn't been a month yet and it's very likely someone is using it

@CaselIT
Copy link
Member Author

CaselIT commented Apr 15, 2021

sure, I was just joking :)

@CaselIT
Copy link
Member Author

CaselIT commented Apr 15, 2021

what method should it call? registry.mapped correct?

@CaselIT CaselIT self-assigned this Apr 15, 2021
@zzzeek
Copy link
Member

zzzeek commented Apr 15, 2021

probably yes

@sqla-tester
Copy link
Collaborator

Federico Caselli has proposed a fix for this issue in the master branch:

Fixed instrument_declarative registry call. https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2752

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@CaselIT CaselIT

Labels
bug Something isn't working declarative has to do with the declarative API, scanning classes and mixins for attributes to be mapped regression something worked and was broken by a change
Projects
None yet
Milestone
1.4.x
Development

No branches or pull requests
3 participants
@zzzeek @sqla-tester @CaselIT