-
Notifications
You must be signed in to change notification settings - Fork 12
Provide a configuration for mapping inventory location #131
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
Comments
👍 I really like this idea. This is a sphinx-specific modification so it'd be nice to keep it contained to the sphinx configuration. Thanks for your work on this lovely package |
To be fair, it is user-facing as well. Depending on what you want your users to see, In any case, much appreciated! Thanks for all the feedback ❤️ |
It seems like the intent is the latter i.e. it should point to the actual code: https://stackoverflow.com/questions/10113892/semantics-of-module |
Alright, the implementation is almost ready, but I hit one complication. Currently we'd need to specify a map like: codeautolink_inventory_map = {
"package.sub.SubBar": "package.SubBar",
"package.sub.SubBar.sub_attr": "package.SubBar.sub_attr",
"package.sub.SubBar.another_attr": "package.SubBar.another_attr",
} which is way more tedious than just specifying codeautolink_inventory_map = {
"package.sub.SubBar": "test_project.SubBar",
} and letting everything else follow. It would be a bit more complicated to implement, but nothing major. Mainly I'm wondering if there are some things that should be taken into account, like would someone ever need (or be able to) specify different locations for things under the same inventory path? I guess so. For example: # package.__init__
from .sub import module
# package.sub.module
class Thing:
pass .. automodule:: package
:imported-members: module
.. autoclass:: package.sub.module.Thing So I'm tempted to play safe and go with the tedious but extra explicit option. Seeing autolink warnings will make enumerating all the objects that should be mapped a bit easier. Thoughts? |
IMO this is OK. It's a one-time pain. |
I also improved the warning messages a bunch to specify why links are missing. Now released as |
Would this be handled by using mypy (#132) or would you still need the manual config? |
I don't expect that to land anytime soon, if at all, since I know nothing about it still. Needs a load of exploration. But I imagine mypy could very well have similar issues with determining the canonical location of a class, one way or the other. |
Actually, this does seem like a major pain. We'd have to add an entry for all of our API which is quite hard. It'd be nice to jsut do
and have everything else follow. |
From xarray-contrib/xarray-tutorial#85: libraries that lift classes up the module hierarchy are now required to override the
__module__
of each class because we use the fully qualified name to figure out the canonical location of a class. In case they don't want to, we could easily provide configuration to map inventory locations to others. For example:The text was updated successfully, but these errors were encountered: