Fix suggested changes from #820 #821
Conversation
flaeppe
force-pushed
the
fix/820-additional-changes
branch
from
1f0975c to
572c64b
Compare
flaeppe
force-pushed
the
fix/820-additional-changes
branch
from
572c64b to
a7cc58d
Compare
| new_manager_info.metadata.setdefault("django", {}) | ||
| new_manager_info.metadata["django"].setdefault("from_queryset_manager", {}) | ||
| new_manager_info.metadata["django"]["from_queryset_manager"] = derived_queryset_fullname | ||
| new_manager_info.metadata["django"] = {"from_queryset_manager": derived_queryset_fullname} |
There was a problem hiding this comment.
I'm guessing this slightly decreases compatibility if some other tool were to use the "django" namespace in the metadata dict?
There was a problem hiding this comment.
Yes, it does. Though the top-level metadata key is set as "django" throughout django-stubs.
There was a problem hiding this comment.
With a quick peek I saw that it's not always the case that "django" is included. Mypy mentions that it's intended for a plugin to have a top-level metadata key coinciding with the plugin name:
Although, I suppose that the key should perhaps be "django-stubs" instead
There was a problem hiding this comment.
Right, I'm probably bike-shedding. Seems highly unlikely that there will be conflicts. And if there are, it can be addressed in the future.
There was a problem hiding this comment.
True, although to perhaps minimize custom metadata usage. It could be useful with some helper to set metadata values(?). Basically just to keep the same top-level name.
def set_metadata_value(key: str, value: Any, info: TypeInfo) -> None:
info.metadata.setdefault("django-stubs", {})
info.metadata["django-stubs"][key] = valueThere was a problem hiding this comment.
For reference, SQLAlchemy's mypy plugin has a utility getter/setter for TypeInfo metadata: https://github.com/sqlalchemy/sqlalchemy/blob/eb716884a4abcabae84a6aaba105568e925b7d27/lib/sqlalchemy/ext/mypy/util.py#L90-L95
Fixes some additional suggested changes in #820. Just minor cleanup really.
Related issues
Refs #820