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

feat: Allow linking to an identifier not only by its exact anchor #217

Merged
merged 4 commits into from
Jan 19, 2021

Conversation

oprypin
Copy link
Member

@oprypin oprypin commented Jan 16, 2021

No description provided.

@oprypin oprypin force-pushed the anylink branch 2 times, most recently from c8d15aa to 39d77f8 Compare January 16, 2021 20:18
@pawamoy
Copy link
Member

pawamoy commented Jan 17, 2021

...but also by? 😄

I'm not sure to get the change here. Basically the URL map is moved into the Handlers instance, with a method to register an anchor, and a method to get an anchor from a collected item, overwritable in each handler. 👍

Then when fixing cross-references, we search if we have a registered anchor for a given identifier, and if we do not, we try to collect the item through each handler to compute the anchor?

I'd like to discuss the case when the anchor is not already registered. The case happens when someone cross-references an object that was not injected in the docs (it was not collected nor rendered). Then how force-collecting it to get its anchor would be useful, as the object is still not rendered in any page? I think I'm missing something 😄

@oprypin
Copy link
Member Author

oprypin commented Jan 17, 2021

The slight mistake in your description is the part about "force-collecting". No such thing is happening, and the object's "linkability" is not affected.

The use case for me: an object has its very strict "canonical" anchor but collect (:::) has the ability to find it through some more "lax" alias, while [][] linking doesn't have that ability, it requires the exact spelling. Maybe you also ran into differences with "spelling" with the new_style thing in Python handler.

@oprypin
Copy link
Member Author

oprypin commented Jan 17, 2021

the object's "linkability" is not affected.

because even if the collector returns an anchor and that anchor doesn't exist, the behavior will be exactly the same as without this change

@pawamoy
Copy link
Member

pawamoy commented Jan 17, 2021

Oh okay I see now. Your change allows to do something like this:

# pkg/a.py
from pkg.b import Kls
# pkg/b.py
class Kls:
    ...
Inject object:

::: pkg.a:Kls

Cross-reference it (in markdown directly or in docstrings),
both are equivalent:

- [my kls][pkg.a.Kls]
- [my kls][pkg.b.Kls]

@oprypin
Copy link
Member Author

oprypin commented Jan 17, 2021

Did a force push because it contained a commit that was merged elsewhere.
It's a no-op, nothing new to review: https://github.com/pawamoy/mkdocstrings/compare/39d77f8..4789950

@oprypin
Copy link
Member Author

oprypin commented Jan 17, 2021

Oh I think you're right in your comment. So then, it fixes #178 (comment) :D I already forgot

@pawamoy
Copy link
Member

pawamoy commented Jan 17, 2021

Well I'd say it's a workaround for #178 and not a full fix, for the following reason: if you import a class from a private module in a module above it, it's because you want to make it public there. Therefore you want that class to use its public path as canonical, not its real path, otherwise it defeats a bit the purpose of documenting your API (and it's the current behavior, that I think is wrong, in pytkdocs). Imagine telling users to inherit from pkg.a.Model, but having that class rendered in pkg/_b with #pkg._b.Model as anchor: not great.

Anyway, not much to do with this PR, I'm mainly commenting on #178 😅

@oprypin
Copy link
Member Author

oprypin commented Jan 17, 2021

Regressions run shows nothing newly broken https://github.com/oprypin/mkdocstrings-regressions/actions/runs/491976653

@pawamoy
Copy link
Member

pawamoy commented Jan 17, 2021

OK, LGTM! Would you also like to update the docs to document this new feature (and its limitation when names clash)? If not, I can do it of course, but not sure when!

@oprypin
Copy link
Member Author

oprypin commented Jan 17, 2021

OK, added the docs :D
No excuse

@pawamoy
Copy link
Member

pawamoy commented Jan 19, 2021

Passing a callable, heh, OK with me 🙂
Thank you for the docs, and the typo fixes 😄

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.

None yet

2 participants