Skip to content

Enhance namespace package repr #98139

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

Closed
iyume opened this issue Oct 10, 2022 · 4 comments
Closed

Enhance namespace package repr #98139

iyume opened this issue Oct 10, 2022 · 4 comments
Labels
type-feature A feature request or enhancement

Comments

@iyume
Copy link
Contributor

iyume commented Oct 10, 2022

Feature or enhancement

After Loader.module_repr has been deprecated, the repr of namespace is like:

<module 'xx' (<_frozen_bootstrap_external.NamespaceLoader object at 0x0000000>)>

which is unreadable. So I purpose the following repr:

<module 'xx' (namespace) from ['path1', 'path2']>

The change purposed to code is:

def _module_repr_from_spec(spec):
    """Return the repr to use for the module."""
    name = '?' if spec.name is None else spec.name
    if spec.origin is None:
        if spec.loader is None:
            return f'<module {name!r}>'
        else:
            return f'<module {name!r} (namespace) from {list(spec.loader._path)}>'   <<< this line changed
            return f'<module {name!r} ({spec.loader!r})>'   >>> the unmodified line
    else:
        if spec.has_location:
            return f'<module {name!r} from {spec.origin!r}>'
        else:
            return f'<module {spec.name!r} ({spec.origin})>'

In my interpreter test:

>>> import a
>>> a
<module 'a' (namespace) from ['/home/iyume/workspace/cpython/a']>

If OK, I can open PR, and add test code.

Previous discussion

@iyume iyume added the type-feature A feature request or enhancement label Oct 10, 2022
@FFY00
Copy link
Member

FFY00 commented Oct 30, 2022

This is tricky, the proposal removes the loader from the repr, which is useful information, and makes it possible to have ridiculously big reprs due to including the search paths, but it does produce arguably better reprs for lots of situations. I am slightly inclined towards keeping the current repr.

@jaraco thoughts?

@hauntsaninja
Copy link
Contributor

I like this feature suggestion, I can think of a couple times it would have helped some people at work.

I'm not sure how intentional showing the loader in the repr was (e.g. 3.9 shows just <module 'xx' (namespace)>), seems mostly that the change to introduce it was to avoid use of the deprecated Loader.module_repr, if possible. Brett was the last person to touch this logic, in case we want his opinion.

@jaraco
Copy link
Member

jaraco commented Nov 6, 2022

IMO, the paths where the namespace is loaded are particularly useful. I'm +1 to the proposal. I'm not sure that I've ever used the repr of the loader except to recognize that it's a namespace package, which is retained in the proposal.

Filipe mentions the risk of ridiculously big reprs, which seems like a plausible scenario, even with just a couple of very long paths. Still, being able to see the paths in the repr seems a lot more valuable than the memory address of the loader.

I'm thinking that almost all uses of this repr are going to be for internal troubleshooting anyway, so I'm not terribly concerned about the size of the repr. I'm not aware of any situations where large reprs could cause problems, though I certainly don't have a full survey of user experiences.

FFY00 pushed a commit that referenced this issue Nov 6, 2022
@FFY00
Copy link
Member

FFY00 commented Nov 6, 2022

Implemented in #98870.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants