Skip to content

DOC: Fix missing-reference generation on Windows #26850

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 4, 2023
Merged

DOC: Fix missing-reference generation on Windows #26850

merged 1 commit into from
Oct 4, 2023

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Sep 21, 2023

PR summary

On Windows, the path is absolute and contains a colon after the drive letter, so splitting on colon results in trying to relativize 'c' (or whatever the drive is) to the base directory, which just makes the final path into base directory + file path.

These missing references all broke in #26628, though I'm not sure why builds never failed since they aren't on Windows, or using the same base paths.

PR checklist

@QuLogic QuLogic added the Documentation: build building the docs label Sep 21, 2023
@QuLogic QuLogic added this to the v3.8.1 milestone Sep 21, 2023
@QuLogic QuLogic changed the title DOC: Fix misssing-reference generation on Windows DOC: Fix missing-reference generation on Windows Sep 21, 2023
@QuLogic
Copy link
Member Author

QuLogic commented Sep 21, 2023

though I'm not sure why builds never failed since they aren't on Windows, or using the same base paths.

Ah, I think because on load and comparison it runs through:

matplotlib/doc/sphinxext/missing_references.py

Lines 135 to 142 in 2cbdff8

def _truncate_location(location):
"""
Cuts off anything after the first colon in location strings.
This allows for easy comparison even when line numbers change
(as they do regularly).
"""
return location.split(":", 1)[0]

which with the Windows-style paths gives us always doc/C so everything matches, perhaps?

@story645
Copy link
Member

The missing references file confuses me/I also I think more or less had the linter ignore it - I'm just not sure which of those I'm supposed to care about.

@QuLogic QuLogic force-pushed the fix-missing-refs branch 3 times, most recently from e1651de to f762b4c Compare September 21, 2023 23:40
@QuLogic
Copy link
Member Author

QuLogic commented Sep 22, 2023

So it looks like CI is getting additional unused references:

doc/docstring of builtins.list:17: WARNING: Reference py:class HashableList[_HT] for doc/docstring of builtins.list:17 can be removed from missing-references.json. It is no longer a missing reference in the docs.
lib/mpl_toolkits/axisartist/axisline_style.py:docstring of mpl_toolkits.axisartist.axisline_style.AxislineStyle:1: WARNING: Reference py:class mpl_toolkits.axisartist.axisline_style._FancyAxislineStyle.FilledArrow for lib/mpl_toolkits/axisartist/axisline_style.py:docstring of mpl_toolkits.axisartist.axisline_style.AxislineStyle:1 can be removed from missing-references.json. It is no longer a missing reference in the docs.
lib/mpl_toolkits/axisartist/axisline_style.py:docstring of mpl_toolkits.axisartist.axisline_style.AxislineStyle:1: WARNING: Reference py:class mpl_toolkits.axisartist.axisline_style._FancyAxislineStyle.SimpleArrow for lib/mpl_toolkits/axisartist/axisline_style.py:docstring of mpl_toolkits.axisartist.axisline_style.AxislineStyle:1 can be removed from missing-references.json. It is no longer a missing reference in the docs.
lib/matplotlib/path.py:docstring of matplotlib.path:1: WARNING: Reference py:class numpy.uint8 for lib/matplotlib/path.py:docstring of matplotlib.path:1 can be removed from missing-references.json. It is no longer a missing reference in the docs.
doc/docstring of builtins.list:17: WARNING: Reference py:obj matplotlib.typing._HT for doc/docstring of builtins.list:17 can be removed from missing-references.json. It is no longer a missing reference in the docs.

but I don't see them locally. I guess that's probably because of some differing versions of things. I'll leave that option off for now.

@story645
Copy link
Member

A possibly good first issue is running through those and commenting out the corresponding ignored_warnings on the linters

@QuLogic
Copy link
Member Author

QuLogic commented Sep 22, 2023

The missing references file confuses me/I also I think more or less had the linter ignore it - I'm just not sure which of those I'm supposed to care about.

This file causes the WARNING: xyz link target could not be found type of things to be skipped. Mostly they're there for stuff that isn't documented, but linked, e.g., a method with an inherited docstring from a superclass with links to other methods that are not overridden (so the linked methods don't show up in the subclass).

@QuLogic
DOC: Fix missing-reference generation on Windows
5e5378e 
On Windows, the path is absolute and contains a colon after the drive
letter, so splitting on colon results in trying to relativize 'c' (or
whatever the drive is) to the base directory, which just makes the final
path into base directory + file path.
ksunden
ksunden approved these changes Sep 27, 2023
View reviewed changes
Copy link
Member

@ksunden ksunden left a comment

Choose a reason for hiding this comment

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

The changes look fine, but I am slightly confused as to why we weren't getting warnings for all of the things in that file that had the windows paths.

Are we ignoring the path portion entirely somehow? if so should it just be removed in total from the json file?

I get that we have config on whether or not to warn about unused entries in this file (which is currently set to not warn for those), but presumably many of these are valid entries that should be included in the ignores.

@story645
Copy link
Member

we weren't getting warnings for all of the things in that file that had the windows paths

Where were you expecting those to show up?

@ksunden
Copy link
Member

ksunden commented Sep 27, 2023

I guess I was expecting the warning that should have been silenced to still show up (and flag as a warning/get extracted by the circle build step etc.

@QuLogic
Copy link
Member Author

QuLogic commented Sep 27, 2023

Are we ignoring the path portion entirely somehow? if so should it just be removed in total from the json file?

I looked again; it appears that the answer is yes. Only the keys are used for ignoring warnings:
https://github.com/matplotlib/matplotlib/blob/2cbdff865f170fffb868cc12f1265715c0e4f40c/doc/sphinxext/missing_references.py#L285C62-L285C62

The file paths appear to be only to help people to investigate ignored things, and to make the warnings for unused ignores.

@story645 story645 closed this Oct 4, 2023
@story645 story645 reopened this Oct 4, 2023
@ksunden ksunden merged commit 58646c2 into matplotlib:main Oct 4, 2023
@meeseeksmachine meeseeksmachine mentioned this pull request Oct 4, 2023
Merged
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Oct 4, 2023
@ksunden @meeseeksmachine
Backport PR matplotlib#26850: DOC: Fix missing-reference generation o…
3e1afb0 
…n Windows
@QuLogic QuLogic deleted the fix-missing-refs branch October 4, 2023 19:16
QuLogic added a commit that referenced this pull request Oct 4, 2023
@QuLogic
Merge pull request #26997 from meeseeksmachine/auto-backport-of-pr-26…
617b775 
…850-on-v3.8.x

Backport PR #26850 on branch v3.8.x (DOC: Fix missing-reference generation on Windows)
@ksunden ksunden mentioned this pull request Nov 2, 2023
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@story645 story645 story645 approved these changes

@ksunden ksunden ksunden approved these changes

Assignees
No one assigned
Labels
Documentation: build building the docs
Projects
None yet
Milestone
v3.8.1
Development

Successfully merging this pull request may close these issues.
3 participants
@QuLogic @story645 @ksunden