bpo-32471: Add class diagram to collections.abc doc by DevOpsCraftsman · Pull Request #5133 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation8 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

DevOpsCraftsman changed the title~~pbo-32471: Add class diagram to collections.abc doc~~ bpo-32471: Add class diagram to collections.abc doc

Jan 7, 2018

Thanks for the PR!

I don't know how to run the doc locally to view how it looks...

this will create a build folder with HTML files for documentation, just open the one you are interested in with your favorite browser.

Please add a NEWS item, you need to install the blurb tool for this (using it is easy, but it only works on Python 3.5+).

Concerning the image I have some comments:

Why __call__ and __hash__ don't have parentheses () next to them?
There is too much italic, I would rather make the class names bold instead of italic.
I would place AsyncIterable/AsyncIterator/AsyncGenerator line near the Iterable etc line.
Also I would place Callable and Awaitable close to each other, they are logically related.

Thank you!

Why __call__ and __hash__ don't have parentheses () next to them?

Forgot these...

There is too much italic, I would rather make the class names bold instead of italic.

As I said in the python-ideas thread where this was discussed, my opinion is to stick to UML standards for those kind of details as most as possible. Everybody has his own taste about it (some proposed to put the concretes methods first, and the abstract ones after), so I think the best approach is to take a opinion-agnostic methodology by following UML conventions.
Plus the tool I used (plantuml) can't deal with that: abstract methods are in italic and I don't think it's possible to change it...

I would place AsyncIterable/AsyncIterator/AsyncGenerator line near the Iterable etc line.

I will commit this change.

Also I would place Callable and Awaitable close to each other, they are logically related.

The layout will suffer of that: it's gonna grown larger... Even the last change you proposed will make it bigger.

Choose a reason for hiding this comment

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

OK, this already looks good to me. Maybe later we can "polish" the layout. I will not merge it yet, maybe @rhettinger has some more comments.

After some waiting time, I think it's a pity to not have this nice addition to the doc, So:

At this day, I don't think that the problem evoked in https://bugs.python.org/issue32621 is that relevant, because the diagram is another thing. I just sicked to the convention: don't re-mention a inherited method unless it has been overridden.

I finally switched to ArgoUML, and included to the commit a .uml witch is basically a kind lf .xml file used for UML diagrams. It can be read by some Eclipse plugins, including ArgoUML.

To be validated by @rhettinger.

EDIT: It can be backported to the 3.6 because the classes involved havn't changed since.

The diagram looks nice.

Two concerns:

The diagram is somewhat wide relative to the surrounding HTML.
We need to be able to maintain the image if there are future additions or modifications to the collection ABCs. How would that be done?

I thought it fit perfectly... It's an SVG, so it adapt itself to the brower's window.

I can think of two things if you really want a workaround for that :

Put the async classes an another diagram under
Make a link to a standalone page where lives the diagram.

About the second concern, as I sais in the previous comment, I used ArgoUML (wich is open source), and included to the commit the .uml file for the diagram witch is basically a kind lf .xml file used for UML diagrams. It can also be read and edited by some Eclipse plugins.

http://argouml.tigris.org/

I showed this to learners in my class and found that it wasn't a net win. The large visual pulled people away from the table that contained all the essential information they needed to write code. I'm thinking now that the relationship between the various ABCs aren't what we want to emphasize. What a user cares about is what methods are required and what is provided. Knowing that Set and Mapping are siblings isn't useful enough to warrant being repeated in the diagram.

Thank you for the suggestion. I had hope that it would be an improvement but didn't find that it was in practice. Do consider publishing it in a blog or somesuch.