Skip to content

feat: Allow resolving alias to external modules#60

Merged
pawamoy merged 1 commit intomkdocstrings:masterfrom
gilfree:load_external_modules
Mar 5, 2023
Merged

feat: Allow resolving alias to external modules#60
pawamoy merged 1 commit intomkdocstrings:masterfrom
gilfree:load_external_modules

Conversation

@gilfree
Copy link
Copy Markdown
Contributor

@gilfree gilfree commented Feb 15, 2023

Hi

I would like to suggest adding an option (false by default) to set griffe's resolve external modules flag to true.

What are your thoughts about it? Did I do it correctly? As I am not a native English speaker, my documentation my need some improvement.

@pawamoy
Copy link
Copy Markdown
Member

pawamoy commented Feb 15, 2023

Hello, thanks for the PR! May I ask what is your use-case, and what you think of this comment mkdocstrings/mkdocstrings#503 (comment)?

@gilfree
Copy link
Copy Markdown
Contributor Author

gilfree commented Feb 15, 2023

Hello, thanks for the PR! May I ask what is your use-case, and what you think of this comment mkdocstrings/mkdocstrings#503 (comment)?

Basically I have a rather large library that is exposed through multiple independent modules with other 'root' names - they do not share a package name (a bad case of need to preserve backward computability).

The use of __all__ is rather limited in this code base (And as I understand is discouraged in general in newer code as its goal is to allow * imports, which are discouraged). This means that I can easily control with __all__ what is pulled by griffe.

I think I can manage with pre loading as you suggested in the issue you have referenced, although it will require a bit more manual work, of finding out which modules to pre-load.

I think I can change this PR to add preload_modules: ['a','b','c'] option instead of load_external_mdules: bool. Would you prefer that I do this change?

From my POV the external and selectively using __all__ gives better ergonomics, but if you prefer explicit preloading, I will change the PR.

@pawamoy
Copy link
Copy Markdown
Member

pawamoy commented Feb 15, 2023 •
edited
Loading

Basically I have a rather large library that is exposed through multiple independent modules with other 'root' names - they do not share a package name (a bad case of need to preserve backward computability).

I see, thanks.

The use of all is rather limited in this code base (And as I understand is discouraged in general in newer code as its goal is to allow * imports, which are discouraged). This means that I can easily control with all what is pulled by griffe.

Yeah, because of the lack of a standard way to "export" objects or make them public, __all__ is hijacked for this, while its original use is for wildcard imports, which are indeed heavily discouraged.

I think I can manage with pre loading as you suggested in the issue you have referenced, although it will require a bit more manual work, of finding out which modules to pre-load.

I think I can change this PR to add preload_modules: ['a','b','c'] option instead of load_external_mdules: bool. Would you prefer that I do this change?

From my POV the external and selectively using all gives better ergonomics, but if you prefer explicit preloading, I will change the PR.

Just to be clear: the preload solution would add a configuration option in the Python handler, and its value would be used as early as possible to load the specified modules in the handler's _module_collection. The workaround described in the comment I linked is... just a workaround.

Both (preload, and external+__all__) are compatible, and I don't mind having a configuration option to allow loading external modules, since you have a use-case for it, and I'm sure others will. I just didn't want to implement it myself (because I wouldn't use it), at least before the preload way is added. So if we are to merge this very PR, I'd like preload to be implemented first, as that would be the recommended way, while the external solution would be documented as "at your own risk".

I'm imagining something like this: starting from scratch, users temporarily allow loading external modules, and at the end of the build, the Python handler outputs an info/debug message suggesting to disable external loading and enable pre-loading with the list of external modules it loaded during the build. Maybe we could even try to exclude modules for which no objects were rendered from this suggested list. But these are nice-to-have, it could be done later.

If you feel like implementing both, that would be awesome! I'd recommend doing it in two distinct PRs. Let me know if that makes sense! In any case this conversation helped me clear my mind and I can see the path forward, so I'd eventually get it to it 🙂

@gilfree gilfree force-pushed the load_external_modules branch from 5516ddf to 3d9eb81 Compare February 15, 2023 20:23
@gilfree
Copy link
Copy Markdown
Contributor Author

gilfree commented Feb 15, 2023 •
edited
Loading

I changed the PR to preload modules. It works good enough for my use case, and I'm ok with that, and with adding external at a different PR.

Let me know if you think that is good enough.

@gilfree gilfree mentioned this pull request Feb 15, 2023
Merged
@gilfree gilfree force-pushed the load_external_modules branch 2 times, most recently from f3f704e to a2bc632 Compare February 15, 2023 20:39
@gilfree
Copy link
Copy Markdown
Contributor Author

gilfree commented Feb 28, 2023

Hi.
I don't want to bother, but just in chance you missed the edits - do you think those (#60, #61) PRs are good enough now? Do you plan to accept them?
Thanks!

@pawamoy
Copy link
Copy Markdown
Member

pawamoy commented Mar 1, 2023

Hello! No worries, pinging me is the right way 🙂

pawamoy
pawamoy reviewed Mar 1, 2023
View reviewed changes
@gilfree gilfree force-pushed the load_external_modules branch from 6bcd7df to b774ff7 Compare March 2, 2023 06:32
pawamoy
pawamoy requested changes Mar 3, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

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

OK, sounds good to me. I checked it out and it seems it's not possible to preload the modules in the __init__ method yet. So lets do it this way for now and maybe we'll refactor later 🙂 Thanks a lot!

@gilfree gilfree force-pushed the load_external_modules branch 2 times, most recently from 53ca4da to fe64e72 Compare March 5, 2023 08:30
@gilfree gilfree requested a review from pawamoy March 5, 2023 08:30
@gilfree
feat: pre load external modules
d9a6326 
Add option to preload external modules that are not
part of the displayed documentation to allow resolving aliases
to objects in these modules, and presenting their documentation
in a module that imports them, and exports the imports with
`__all__` attribute.
@gilfree gilfree force-pushed the load_external_modules branch from fe64e72 to d9a6326 Compare March 5, 2023 08:32
pawamoy
pawamoy approved these changes Mar 5, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@pawamoy pawamoy merged commit 36002cb into mkdocstrings:master Mar 5, 2023
@gilfree gilfree deleted the load_external_modules branch March 6, 2023 10:00
pawamoy pushed a commit that referenced this pull request Mar 7, 2023
@gilfree
feat: Allow resolving alias to external modules
02052e2 
PR #61: #61
Follow-up of PR #60: #60
Co-authored-by: gilfree <gilfree@users.noreply.github.com>
viktorlashchuk added a commit to viktorlashchuk/mkdocstrings-python that referenced this pull request Feb 24, 2025
@viktorlashchuk
feat: Allow pre-loading modules
fc59f03 
Add option to preload modules.
Preloading modules allows to render members of objects
that originate from other packages than their parent.
Direct members of modules must be listed in `__all__`.

This option typically allows to render aliases,
by collecting their parent module (package)
which allows to resolve these aliases.

Issue mkdocstrings/mkdocstrings#503: mkdocstrings/mkdocstrings#503
PR #60: mkdocstrings/python#60
viktorlashchuk added a commit to viktorlashchuk/mkdocstrings-python that referenced this pull request Feb 24, 2025
@viktorlashchuk @gilfree
feat: Allow resolving alias to external modules
bbcdfbe 
PR #61: mkdocstrings/python#61
Follow-up of PR #60: mkdocstrings/python#60
Co-authored-by: gilfree <gilfree@users.noreply.github.com>
@JamesWrigley JamesWrigley mentioned this pull request Jan 16, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pawamoy pawamoy pawamoy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gilfree @pawamoy