-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Improve typing and MIME hook API for inspector #14342
Conversation
Thanks, that is great, it is something I wanted to do for quite some time but needed to find the time to do it. I was envisioning a DataClass, but a typedict is great ! I'll try to get to that and merge it this week |
We might decide to deprecate it separately in another PR
name: str | ||
|
||
|
||
info_fields = list(InfoDict.__annotations__.keys()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if this is techincally public API, I think it's fine to remove as I can't find any usage. Or, we introduce a module level __getattr__
, and emit a PendingDeprecationWarning.
def object_info(**kw): | ||
"""Make an object info dict with all fields present.""" | ||
infodict = {k:None for k in info_fields} | ||
infodict = {k: None for k in info_fields} | ||
infodict.update(kw) | ||
return infodict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make this a InfoDict ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I see that the place where this is used does not set all the fields as found=False
. I guess in a subsequent PR we can also set mandatory field to a proper default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. Also, typing kw
requires Unpack
which was only added in 3.11 (though it can be imported from typing_extensions
).
res = hook(obj, info) | ||
if res is not None: | ||
bundle[key] = res | ||
if self.mime_hooks: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why the if ? Just if the dict is empty ?
If the if dict is empty wouldn't the foor loop just loop over nothing ?
Is it to avoid creation of a useless hook_data if there is no mime_hooks ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it to avoid creation of a useless hook_data if there is no mime_hooks ?
Yes, that was my intention.
Ok, let's get that in and maybe tweak the behavior in subsequent PRs if we decide to. |
Fixes #14339
Additions
Adds
InfoDict
type to improve the typing ofinfo()
result.Adds missing
"subclasses"
toinfo_fields
list (these were added to the field list in #11486 but we forgot to updateinfo_fields
variable at the time) - the newly addedInfoDict
type will ensure that this won't happen again.Adds
InspectorHookData
dataclass which is passed to the MIME hooks which now should expect a single argument. Having a single dataclass argument enables us to deprecate individual fields, or add new fields without breaking the existing hooks. The old hooks will still work (if any are out there since this mechanism got just added in the previous point version).Deletions
A comment over
info_fields
gets deleted:info_fields
were not defining the order of display since at least 2015 (Rearrange info display to put most useful fields first #7903 - I did not feel the need to go further in the history to find when exactly it happened).info_fields
(I guess this was lost during IPython/Jupyter split), but the newly addedInfoDict
at least properly annotates their type (if you know where I can find the old IPython messaging spec with the descriptions I can add these as doc comments).Unused
cast_unicode
import gets deleted. If someone imported it from here... well they really should not have.Deprecations
obj, info
)