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

Link partially qualified identifiers when documenting single module #544

Merged
merged 18 commits into from
Sep 10, 2023

Conversation

Crozzers
Copy link
Contributor

Currently, to link to another identifier using pdoc, the item in question must either be part of the current file or you must give the fully qualified name, including the module name.

This is very inconvenient when documenting a single module with a long name. For example:

# my_long_module_name/__init__.py
from .helpers import something

def a():
     '''This will not be linked: `helpers.something`'''
    pass

def b():
     '''This will be linked: `my_long_module_name.helpers.something`'''
    pass

# my_long_module_name/helpers.py
def something():
    pass

This PR changes the possible_sources function in render_helpers.py to check if every module name in all_modules starts with the same top-level package name. For example, the following list shares the top-level module name of my_long_module_name:

my_long_module_name
my_long_module_name.helpers
my_long_module_name.types
my_long_module_name.types.abc

If a common, global package name can be identified and the identifier does not start with this package name, the package name is prefixed to the identifier.
This means that helpers.something from earlier will become my_long_module_name.helpers.something, allowing the a() function to have the intended link put in.

This does change how some existing code is rendered. The demopackage._child_c.C docstring in the demopackage test used to say ...inherits from .child_b.B but now will be auto-linked as ...inherits from .<a href="#B">demopackage.B</a>.
This docstring doesn't contain any links so it seems like a bug that it's being linkified in the first place, but I'd like to know your thoughts.

@Crozzers
Copy link
Contributor Author

This docstring doesn't contain any links so it seems like a bug that it's being linkified in the first place, but I'd like to know your thoughts.

Regarding this, I created a new branch from main and modified a docstring in the demopackage.child_c submodule in this commit: Crozzers@6f01c47

Here is the new docstring:

# test/testdata/demopackage/child_c.py
class C(child_b.B):
    """This class is defined in .child_c and inherits from .child_b.B

    Here's an identifier that isn't enclosed in back ticks: demopackage.child_b.B
    """

    def c(self):
        return 2

And this is what pdoc generated:
image

From what I can see, linkify is being called from a macro in the module.html.jinja2 template, but I haven't narrowed it down any further.

I'm not sure if this is intentional behaviour, since the pdoc API docs state that you can enclose references in backticks, not that you must. However, it is certainly unexpected

Copy link
Member

@mhils mhils left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR and sorry for letting it sit so long! 🙈
Honestly I'm not a big fan of this change - I feel this muddies the waters on what an absolute and what a relative identifier is. I really appreciate your work on markdown2 though, so I'd like to accomodate your use case if somehow possible.

As an alternative, how would you feel about making relative links start with . or ..? For example, you'd write Do `.helpers.something` with a `.types.BigThing`.. That'd be a bit more complex to implement, but at least it's not ambiguous.

pdoc/render_helpers.py Outdated Show resolved Hide resolved
@Crozzers
Copy link
Contributor Author

Thank you for the PR and sorry for letting it sit so long

No worries! Apologies for the delay in getting back to you.

As an alternative, how would you feel about making relative links start with . or ..? For example, you'd write Do .helpers.something with a .types.BigThing.

Yep, that makes alot more sense actually. I've updated the branch to only link partially qualified names if the name is prefixed with . and if all possible sources are within the same module.

Crozzers added a commit to Crozzers/screen_brightness_control that referenced this pull request Jul 9, 2023
@Crozzers Crozzers requested a review from mhils July 23, 2023 14:21
@mhils
Copy link
Member

mhils commented Jul 24, 2023

Thanks for keeping this up-to-date. I don't have the capacity to review this thoroughly at the moment, but I will get back to this once I have a bit of time for pdoc again. :)

@mhils
Copy link
Member

mhils commented Sep 6, 2023

Thanks for waiting so patiently! I've now had a chance to look at the implementation more closely and tweak some things. Let me know if that looks good from your end, and I'll be happy to merge things and ship a pdoc release right after. 😃

@Crozzers
Copy link
Contributor Author

Crozzers commented Sep 6, 2023

Is the intention to only do relative links if the link starts with ..? Or is .. used to resolve a parent module?

There are a few instances in the demopackage where single dot refs are made but no link occurs. For example, in demopackage.child_c it says:

This class is defined in .child_c and inherits from .child_b.B

But no links are made.
image

Debugging this, when the code gets to linkify_repl, the mod.modulename is 'demopackage.child_c' which results in an identifier of 'demopackage.child_c.child_b.B'.

It seems that . is referring to items in the same module, though I assumed it would refer to adjacent submodules.

Edit: Perhaps stripping the current module name from the modulename when relative IDs are used would address this?

@mhils
Copy link
Member

mhils commented Sep 6, 2023

Is the intention to only do relative links if the link starts with ..? Or is .. used to resolve a parent module?

If we are in module foo.bar.baz and see a link to .qux.Quux, that should refer to foo.bar.qux.Quux. ..qux.Quux should refer to foo.qux.Quux. So .. is used to resolve the parent module.

I'll look into the issues you mentioned momentarily. :)

@mhils
Copy link
Member

mhils commented Sep 6, 2023

Thanks! Your review caught an importan bug, I was testing from __init__.py only, where .foo should refer to a child and not an adjacent module (matching how imports work).

Looking better now?

@Crozzers
Copy link
Contributor Author

Crozzers commented Sep 9, 2023

Hi, apologies for slow reply.

I found another issue, where imported classes with relative docstrings don't link properly in the module that imports them. EG:
image
For the imported class B, the parent_module is set as demopackage.subpackage, so it cannot find .child_b within that module.

Currently trying to figure out if there's a way to tell what member the current docstring belongs to so that the parent module can be properly adjusted.

EDIT: my quick fix looked something like this:

            if mod.is_package and not mod.modulename.rpartition(".")[0] + identifier in [i[0] for i in mod.inherited_members]:
                # If we are in __init__.py, we want `.foo` to refer to a child module.
                parent_module = mod.modulename
            else:
                # If we are in a leaf module, we want `.foo` to refer to the adjacent module.
                parent_module = mod.modulename.rpartition(".")[0]

Obviously not ideal but I couldn't see how to trace which object was currently being documented and where it came from.

Also added a fix for the ..child_b.B that wasn't being linked in the picture

@mhils
Copy link
Member

mhils commented Sep 9, 2023

Thanks for the thorough testing! This is a really useful collaboration. 🍰
I've just pushed for the issue in your last comment + some nits. Let me know how that looks to you, and no worries on (not even) slow replies! 😊

@Crozzers
Copy link
Contributor Author

All the links in demopackage seem to be linking to all the right places. Looks good to me! Thank you so much for your help on this one

@mhils mhils merged commit c49da24 into mitmproxy:main Sep 10, 2023
12 checks passed
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.

2 participants