-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
bug: "regression" in 1.1.0 regarding "Multiple URLs found for" #52
Comments
Thanks for the report! I was expecting to get one but still thought this warning had to be enabled by default. I definitely didn't anticipate this use-case you have though.
That's exactly the issue the warning is aiming to solve. With non-unique heading ids across pages, cross-references might link you to one or the other, non-deterministically (or deterministically, but depending on the order the pages are built, and this might be more complex than that). I agree that this could be a feature request indeed: the feature I think we need here is a way to change the heading ids of the ones if your Guide pages. By the way, I can't seem to find any mkdocstrings-generated headings in your Guide pages, could you link me to some examples? I'd like to see how you render them. EDIT: I see subheadings such as It might be possible to workaround this right now, by doing the following:
|
Sorry, I don't know what this means. 😞, so I don't know how to point you to examples. Not sure if you still need them given the EDITs.
Yeah, no, sadly it doesn't. We are even using |
@llucax I'd like to inspect a bit more your docs sources, is there a particular branch I should look at? One that triggers the warnings. I can't seem to find the sources for https://frequenz-floss.github.io/frequenz-sdk-python/v1.0-pre/user-guide/formula-engine/. I don't think the gen-files script generates that too, right? |
The default branch should have the issue. We just pinned the dependency to avoid CI failures until we can find a solution, but if you manually update the dependency it should break. All non-generated docs are in https://github.com/frequenz-floss/frequenz-sdk-python/blob/v1.x.x/docs/user-guide/microgrid-concepts.md also have the issue. Thanks a lot for looking at it! ❤️ |
Thanks, I wasn't looking into the right repo. So, by adding the following options:
(which you already have in actors and microgrid-concepts) ...that reduces the number of warnings. The four remaining ones come from subheadings in the corresponding objects' docstrings ( A few solutions that come to mind:
Obviously, we could also consider reverting, and putting these warnings behind an option. But that means a lot of maintenance, to maintain incorrect/un-deterministic behavior a few projects rely on. If many projects are impacted by this and come here to add their voice, we'll do something, but for now it seems only two projects are impacted. WDYT? Would an option to configure (sub)headings prefixes work for you? I imagine something like this: # Actors
::: frequenz.sdk.actor
options:
members: []
headings_prefix: ""
show_bases: false
show_root_heading: false
show_root_toc_entry: false
show_source: false There's an alternative solution by the way. You could use pymdownx.snippets instead of mkdocstrings to inject your docstrings into your "Guide" Markdown pages. See https://facelessuser.github.io/pymdown-extensions/extensions/snippets/#snippet-sections. Snippets lets you declare sections in your sources, whathever the language (and comment syntax). Then you can inject just these sections into pages. In # License: MIT
# Copyright © 2022 Frequenz Energy-as-a-Service GmbH
# Please note this docstring is included in the getting started documentation in docs/.
"""Actors are a primitive unit of computation that runs autonomously.
<!-- --8<-- [start:docstring] -->
## Actor Programming Model
...
<!-- --8<-- [end:docstring] -->
""" I'm using HTML comment syntax since we're in a Markdown docstring. Note that the docstring summary is not included in the snippet section because it would be weird. So here you'd have to repeat just this summary in the Markdown page: # Actors
Actors are a primitive unit of computation that runs autonomously.
--8<-- "src/frequenz/sdk/actors/__init__.py:docstring" Well, it's not perfect, just wanted to list this alternative 🙂 |
Hi, thanks again for having a deep look at this. I also agree that just disabling the warning is not ideal, as there is a potential issue, it would be much better to have a way to disambiguate something ambiguous instead. About the snippets solution, I prefer to avoid it, another goal of us is to keep docstrings as readable as possible, same reason we try to keep most docs in docstrings, they are accessible via the IDE and code editors, which is in our experience the main way our users look at docs. So your proposed solution of So we are currently linking to that "The run() method" using: [_run]: #the-_run-method In this case, if we leave the link as is it will point to the reference, and if we change it to I guess we can live with that, but ideally the link should resolve "locally" when possible, it would be best if it resolved to the user guide when rendered in the user guide and to the reference when rendered in the reference. This is why I was thinking if it would be possible to have some sort of "resolve multiple URL using this page" kind of option I suggested before. So in this case mkdocstrings will find 2 locations for
So it will use the link to that page without giving any warnings. I can imagine it might be a bit of work and maybe it is not justified if we one of a kind using this documentation scheme, but it would be good to know if that would be even doable :) |
Completely agree 👍
That's the idea 🙂
Yes!
Then you shouldn't use autorefs for this. Autorefs will always resolve references to one and only one location, be it on the same page or somewhere else. Instead, you should use the native reference syntax
Such a solution would have to be implemented in autorefs, but would only be supported through mkdocstrings, which would be a bit weird. Autorefs will definitely have more features in the future (and this might be one), but we're not ready for that yet. We need solid foundations for this type of features first (there's some work going in that direction with #46) 🙂 |
Actually, even prefixing ids might be complicated to implement 🤔 If |
I see, so if all the anchors pointing to symbols are fixed by using:
(not sure if the later is desirable or not, will need to check it out) And for regular anchors we can avoid autorefs and it does exactly what we want (resolve locally) without any warnings, I think that's all we need. I'm not sure why we used autorefs there, but I wouldn't surprised if it was just a mistake. I will try it out.
If the above works out, then I don't think I need prefixes at all. I'm not sure if we are using deep linking to link into a section of a docstring, but if we do I guess we could live without those. Will try to give this a try (next week) and report back. Once again, thanks a lot for your support! ❤️ |
Possibly for consistency. For example, the It's possible that there are places where you cannot use a "local reference" (direct link using # instead of autorefs) because the link is not rendered on the same page, and using an autoref will emit warnings. So you might find yourself stuck again 😕
Yes please, don't hesitate to share your progress with me, I'll gladly chime in again and try to help 😄 I'll leave this open in the meantime. |
OK, I'm having a look at this and now I know why we used the
So in this case the intention here was to explicitly define the link, which happens to point to a HTML fragment/ID instead of a full URL. Is there any way for autorefs to detect when we are actually explicitly defining where On the other hand, we do are using some autoref links for "deep linking" into some other sections:
This doesn't seem to be fixed by just adding:
(these options are actually already there) For this former we could get around with removing the So it seems all our problems are not solved by the solution above, and some are only partially solved, producing worse quality docs (less precise linking). I'll await in case you have any new ideas, and otherwise I guess we'll have to link to modules instead of module's sections and just tell the user "go find this section inside that doc yourself" basically, which I guess it kind of goes against the goals of this tool (to make the best docs ever possible :) ). I created a draft PR with my attempt to solve this: I would be awesome if the |
Good news, commit b0996a3 shouldn't be necessary. Bad news, I'm not sure why links weren't rendered naturally. Let me explain. autorefs only tries to fix references that weren't already rendered naturally. All these Second commit is what I expected, unfortunately. Generally speaking, rendering the same contents in two different places, and the same autorefs linking to this contents twice too, makes it impossible to not get the warnings. Well at least autorefs is not equipped to deal with this. The only way to fix this would be to update the link syntax to be able to provide additional metadata to help decide how to resolve the link (in addition to a way of specifying id prefixes, as previously discussed). |
Another option, probably much more attainable in the short term, would be to add an option to mkdocstrings to say "don't register any heading generated here against autorefs", similarly to mkdocstrings/mkdocstrings#671 and mkdocstrings/mkdocstrings#672. EDIT: Well it seems it's covered by mkdocstrings/mkdocstrings#671 already. But maybe both aspects (autorefs and inventory) should be made configurable distinctly. |
I tried again in your repo, and the |
With or without this commit, I only get three seemingly unrelated warnings (that I cannot trace to their origin, but it's another story). EDIT: OK it's because I had the previous code still installed in the venv. No warnings now, even with commit b0996a3 reverted. |
* Bump the python-dependencies group with 3 updates Bumps the python-dependencies group with 3 updates: [fastapi](https://github.com/fastapi/fastapi), [jarvis-tools](https://github.com/usnistgov/jarvis) and [mkdocs-material](https://github.com/squidfunk/mkdocs-material). Updates `fastapi` from 0.112.1 to 0.112.2 - [Release notes](https://github.com/fastapi/fastapi/releases) - [Commits](fastapi/fastapi@0.112.1...0.112.2) Updates `jarvis-tools` from 2024.5.10 to 2024.8.10 - [Release notes](https://github.com/usnistgov/jarvis/releases) - [Commits](https://github.com/usnistgov/jarvis/commits) Updates `mkdocs-material` from 9.5.31 to 9.5.33 - [Release notes](https://github.com/squidfunk/mkdocs-material/releases) - [Changelog](https://github.com/squidfunk/mkdocs-material/blob/master/CHANGELOG) - [Commits](squidfunk/mkdocs-material@9.5.31...9.5.33) --- updated-dependencies: - dependency-name: fastapi dependency-type: direct:production update-type: version-update:semver-patch dependency-group: python-dependencies - dependency-name: jarvis-tools dependency-type: direct:production update-type: version-update:semver-minor dependency-group: python-dependencies - dependency-name: mkdocs-material dependency-type: direct:production update-type: version-update:semver-patch dependency-group: python-dependencies ... Signed-off-by: dependabot[bot] <[email protected]> * Pin mkdocs-autoref for now (see mkdocstrings/autorefs#52) * s/autoref/autorefs/g --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Matthew Evans <[email protected]>
A lot was written here, let me try to summarize.
WDYT? |
Thanks for the summary, I was a bit busy and didn't have time to go through all of it properly. The last 2 points sound very reasonable to me, and it seems like they should cover all our use-cases. The "local resolution" seem like the most important to me, it should bring us on par to what we had before. The second one would be a nice add-on, and I think it could actually improve our current documentation, as we could avoid adding some undesirable self-references in the user guide and just always point the users to the reference. I will update the draft PR to remove the unnecessary commit when I have some time. Thanks for checking that! |
Not really relevant, but there's another change coming up that can potentially make CI fail, see our announcement here: #56. Users who saw their CI fail because of the duplicated headings warnings and who pinned mkdocs-autorefs to avoid latest version might want to wait or test the upcoming version before unpinning/upgrading 🙂 I'll let you know when the change is merged, and again when it is released. |
Thanks for the heads up, I think (and hope!) we will not be impacted by that one. I updated my draft PR and I can confirm that the changes to |
Do you have any ideas of a time frame for implementing the local references, if you plan to do it eventually at all? Just to know if I should wait for a solution or if I should merge that PR as it and live with the "degraded" documentation for now. |
I'm currently focusing on mkdocs-autorefs so these changes and features will be released in the upcoming week(s). |
OK, just pushed the "resolve closest" feature commit. The option is false by default, because I don't want to encourage its use, since it can be problematic for object inventories. plugins:
- autorefs:
resolve_closest: true See https://github.com/mkdocstrings/autorefs?tab=readme-ov-file#non-unique-headings. Just waiting to see how CI behaves, and I'll publish a new release 🙂 |
I updated the PR and using the new option worked flawlessly! Also no regressions detected with 1.2.0 🎉 Thanks a lot for taking care of this ❤️ |
This PR unpins the `mkdocs-autorefs` plugin, so we can use the latest version, but to do that we need to make some changes to the way we link to sections in the docs. The new version produces warnings when there are more than one possible URL to resolve an "autoref" link, and since we have strict more enabled, the docs fail to build if we have any warnings. The simplest way to avoid the warning in some cases is just adding the following options: ``` show_root_heading: false show_root_toc_entry: false ``` This avoids anchors being created in the user guide, so it removes the ambiguity of the link. To remove some other warnings we add a new `autorefs` plugin option to `resolve_closest`, which allows to resolve the reference to the "closest URL". See the documentation for more information: https://github.com/mkdocstrings/autorefs#non-unique-headings. This PR was done following suggestions in: * mkdocstrings/autorefs#52
This PR unpins the `mkdocs-autorefs` plugin, so we can use the latest version, but to do that we need to make some changes to the way we link to sections in the docs. The new version produces warnings when there are more than one possible URL to resolve an "autoref" link, and since we have strict more enabled, the docs fail to build if we have any warnings. The simplest way to avoid the warning in some cases is just adding the following options: ``` show_root_heading: false show_root_toc_entry: false ``` This avoids anchors being created in the user guide, so it removes the ambiguity of the link. To remove some other warnings we add a new `autorefs` plugin option to `resolve_closest`, which allows to resolve the reference to the "closest URL". See the documentation for more information: https://github.com/mkdocstrings/autorefs#non-unique-headings. This PR was done following suggestions in: * mkdocstrings/autorefs#52
Great! Thanks for your help and patience! |
Warning
This report might look like:
After writing all of this, this probably should have been a feature request instead...
TL;DR: Could an option be added to disable the "Multiple URLs found for" warning and just let references to silently resolve locally as before and/or have an option to resolve links with multiple URLs found to a specific page?
Description of the bug
After doing an update from 1.0.1 to 1.1.0 I started to get many warnings in the form:
And since we are using strict mode, this means an error and docs that won't build.
To Reproduce
The thing is we are using this pattern to try to have as much documentation as possible as part of docstrings, but still have a nice and more organized "user guide": we use some internal modules docstrings as the user guide section and then include it in the markdown documentation. For example:
And we also generate API reference documentation using some script based on what's suggested in the docs, which will create a file like:
Hence, we have the same anchor in 2 different pages.
Expected behavior
Before this upgrade all worked as more or less fine, links just being resolved locally when possible, and externally when not.
But I think things could be even improved for this use case, as before in the example above
FormulaEngine
in the user guide will resolve to the user guide, but ideally it should resolve to the API reference.Add an option to ignore multiple URLs for warnings.
a. Globally in
mkdocs.yaml
b. Locally in the `:: x.y.z`` block
Example:
# Formula Engine ::: frequenz.sdk.timeseries.formula_engine.FormulaEngine options: members: None show_bases: false show_root_full_path: false show_source: false disable_multiple_urls_warning: true
A way to tell where to resolve multiple references to a specific page. For example:
For example something like:
# Formula Engine ::: frequenz.sdk.timeseries.formula_engine.FormulaEngine options: members: None show_bases: false show_root_full_path: false show_source: false resolve_multiple_urls: "frequenz.sdk.timeseries.formula_engine.FormulaEngine": reference/frequenz/sdk/timeseries/formula_engine.md
Additional context
mkdocs-autorefs
updates frequenz-floss/frequenz-sdk-python#1043The text was updated successfully, but these errors were encountered: