Skip to content

More fully qualified argument types #26334

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

oscargus
Copy link
Member

PR summary

PR checklist

@anntzer
Copy link
Contributor

anntzer commented Jul 17, 2023

FWIW I find these rather noisy and would much rather use the short form as much as possible (just like you write from matplotlib.collections import LineCollection instead of writing matplotlib.collections.LineCollection throughout the code). If a user does not know what a LineCollection is, it seems unlikely that just spelling out the fully qualified name is going to help much with understanding the type hint.

@jklymak
Copy link
Member

jklymak commented Jul 17, 2023

That or don't put the "~" in, which suppressed the full path in the html docs?

I actually think it's useful to spell these out, otherwise how does the user know how to spell the import?

@anntzer
Copy link
Contributor

anntzer commented Jul 17, 2023

I actually think it's useful to spell these out, otherwise how does the user know how to spell the import?

If reading the source code: check for the corresponding import at the top of the file (just like you would do if you see LineCollection in the source code).
If reading the html docs: click on the link and see where it takes you.

@jklymak
Copy link
Member

jklymak commented Jul 17, 2023

I think a fair bit of docs get read using an IDE, in which case .PathEffects is not that useful for figuring out what to import. OTOH I imagine it's easy enough to google "matplotlib PathEffects" and figure it out.

I agree that clicking through in html is fine.

@timhoffm
Copy link
Member

Th proposed change reflects our current policy: https://matplotlib.org/devdocs/devel/document.html#reference-types

I don't have a strong opinion on the plain text version. But if you want to change, the policy needs an update.

@oscargus
Copy link
Member Author

I moved quite a lot of files from #26326 to here.

Yes, this is what the docs say.

@timhoffm
Copy link
Member

We have 248 occurences of : `~, and 116 occurences of : `. in comments. So both variants are in place in significant numbers.

I'm inclined to merge this. However, there are a few cases in which the fully qualied name introduces wrapping into the docstring, which is quite disruptive and IMO isn't worth it.

@jklymak
Copy link
Member

jklymak commented Jul 19, 2023

I don't have a strong opinion - just pointing out that full names are sometimes easier to parse, but I guess it depends how people usually use the docs.

@oscargus
Copy link
Member Author

There are also two cases starting with matplotlib (no ~), 22 cases starting with a capital letter (although some are fixed here), and eight (without ~) plus four (with ~) cases where the module is unnecessarily provided.

For the HTML-version it is of course better with a short version, while when reading the source (and not being that used to the code base), the full path makes sense. I guess that is why the documentation states what it does.

@anntzer
Copy link
Contributor

anntzer commented Jul 20, 2023

(I don't like fully qualifying the names everywhere in the docs, but won't block that if there's general agreement to do so.)

@timhoffm
Copy link
Member

(I don't like fully qualifying the names everywhere in the docs, but won't block that if there's general agreement to do so.)

We currently don’t do it everywhere. Fully qualified is only recommended for parameter type descriptions.

I’m inclined to loosen the policy on this to “Use full references, but shorten the reference if this prevents a line break”. That way, we have the more precise info where it doesn’t really hurt legibility, and we don’t make the longer type descriptions even more busy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants