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

Improve typing and MIME hook API for inspector #14342

Merged
merged 2 commits into from Feb 20, 2024

Conversation

krassowski
Copy link
Member

@krassowski krassowski commented Feb 17, 2024

Fixes #14339

Additions

Adds InfoDict type to improve the typing of info() result.

Adds missing "subclasses" to info_fields list (these were added to the field list in #11486 but we forgot to update info_fields variable at the time) - the newly added InfoDict 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:

  • Contrarily to the comment (which is getting deleted in this PR), 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).
  • Also contrarily to this comment, current Jupyter messaging spec does not define the contents of info_fields (I guess this was lost during IPython/Jupyter split), but the newly added InfoDict 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

  • mime hooks taking two arguments (obj, info)

@krassowski krassowski added this to the 8.22 milestone Feb 17, 2024
@Carreau
Copy link
Member

Carreau commented Feb 19, 2024

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

IPython/core/oinspect.py Outdated Show resolved Hide resolved
We might decide to deprecate it separately in another PR
name: str


info_fields = list(InfoDict.__annotations__.keys())
Copy link
Member

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
Copy link
Member

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 ?

Copy link
Member

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.

Copy link
Member Author

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:
Copy link
Member

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 ?

Copy link
Member Author

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.

@Carreau
Copy link
Member

Carreau commented Feb 20, 2024

Ok, let's get that in and maybe tweak the behavior in subsequent PRs if we decide to.

@Carreau Carreau merged commit 2084e7f into ipython:main Feb 20, 2024
18 of 21 checks passed
Carreau added a commit to Carreau/ipython that referenced this pull request Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More complete mime hook API for inspector?
2 participants