bpo-46204: graphlib documentation: general improvements by ctrl-z-9000-times · Pull Request #30269 · python/cpython

ctrl-z-9000-times · 2021-12-26T16:31:01Z

This PR cleans up the documentation for the graphlib module.

I rephrased for grammar and clarity but I did not change any of the semantics.

https://bugs.python.org/issue46204

AlexWaygood · 2021-12-28T14:34:42Z

I have added the skip-news label, since this is a docs-related PR, but I think this should be linked to a BPO issue, since there are some fairly substantial changes being proposed in this PR 🙂

ctrl-z-9000-times · 2021-12-30T13:46:53Z

Ok, I opened a BPO issue for this PR.

merwok

Thanks for your efforts to improve the docs! I have added some remarks and questions, and will also add a general question on the bug report.

merwok · 2021-12-31T18:29:18Z

Doc/library/graphlib.rst

-      Raises :exc:`ValueError` if called without calling
-      :meth:`~TopologicalSorter.prepare` previously.
+      Raises :exc:`ValueError` if this method is called before
+      :meth:`~TopologicalSorter.prepare`.


The previous text seems clear to me.

merwok · 2021-12-31T18:30:01Z

Doc/library/graphlib.rst

+      there are still nodes ready to be returned by
+      :meth:`~TopologicalSorter.get_ready` or there are nodes which were
+      returned by :meth:`~TopologicalSorter.get_ready` and which have not yet
+      been marked as :meth:`~TopologicalSorter.done`.


What is the problem with displaying the full name for these methods? It makes it clear that we’re talking about methods not functions for example.

It's too verbose. The phrase TopologicalSorter.get_ready() appears twice in the sentence.
IMO it should be obvious to the users that get_ready and done are methods, especially since they're clickable links.

merwok · 2021-12-31T18:30:27Z

Doc/library/graphlib.rst

-   before vertex v in the ordering. For instance, the vertices of the graph may
+   A topological order is a linear ordering of the nodes in a graph such that
+   for every directed edge u -> v from node u to node v, node u comes
+   before node v in the ordering. For instance, the nodes of the graph may


This is a substantial change that should be reviewed by the original author of the module and/or doc.

In this context, nodes and vertices are synonyms.
The word "node" is used everywhere else in the document except for this sentence.

merwok · 2021-12-31T18:31:16Z

Doc/library/graphlib.rst

-   ordering is possible if and only if the graph has no directed cycles, that
-   is, if it is a directed acyclic graph.
+   ordering is possible if and only if the graph is a directed acyclic graph,
+   that is, if there are no circular dependencies among the tasks to be performed.


I feel that tasks to be performed is one specific application of graphs, but not a generic term enough to fit in this doc.

If only that were true...
Alas it is clearly stated that the primary purpose of this class is to do task dependency resolution.

The class is designed to easily support parallel processing of the nodes as they become ready.

merwok · 2021-12-31T18:32:18Z

Doc/library/graphlib.rst

+
+      Raises :exc:`CycleError` if any cycles are detected,
+      but :meth:`~TopologicalSorter.get_ready` can still be used to obtain as
+      many nodes as possible until cycles block more progress.


IMHO this change moves lines around without improving the substance.

merwok · 2021-12-31T18:33:21Z

Doc/library/graphlib.rst

   .. method:: static_order()

-      Returns an iterator object which will iterate over nodes in a topological
+      Returns an iterator object which iterates over nodes in a topological


What error is being fixed here?

merwok · 2021-12-31T18:33:35Z

Doc/library/graphlib.rst


-
-      If any cycle is detected, :exc:`CycleError` will be raised.
+      Raises :exc:`CycleError` if any cycles are detected.


Why change this?

For consistency with the rest of pythons docs.
The error messages are usually written in the format:
Raises MyError if my-problem is encountered.

Also, I changed it to use the active voice instead of the passive voice.

I don’t think the original phrasing was incorrect or misleading or problematic in any way.
The Python codebase is old and made by many contributors. Not everything is fully consistent, and that is not a goal.

Yea its correct, but its still poorly written.

The current phrasing puts the most important word "raises" at the end of the sentence.
A reader who is skimming through the page is going to have to read all the way to the end of the sentence to see what the verb is.

The new phrasing starts with the word "Raises" so that before you've even read the sentence, you already know what topic its going to describe.
The new phrase is shorter and easier to comprehend.

merwok · 2021-12-31T18:34:56Z

Lib/graphlib.py


    def done(self, *nodes):
-        """Marks a set of nodes returned by "get_ready" as processed.
+        """Marks a set of nodes returned by "get_ready()" as processed.


I notice that this docstring (and some others) do not follow the common dosctring convention that verbs should be imperative (Mark), not indicative. Would you mind changing them?

merwok · 2021-12-31T18:36:37Z

Lib/graphlib.py

-        number of nodes marked "done" is less than the number that have been returned
-        by "get_ready".
+        Progress can be made if cycles do not block the resolution and either
+        there are still nodes ready to be returned by "get_ready()" or there


Developers spend a lot of time looking at history, to find who changed some line when and for what reason.
Preserving the usefulness of that history is more important that strict line lengths, so here for example I would not rewrap the lines.

merwok · 2021-12-31T18:38:44Z

Lib/graphlib.py

+        been marked as "done()".

-        Raises ValueError if called without calling "prepare" previously.
+        Raises ValueError if called before "prepare()".


This is personal, but I don’t like adding parentheses to all function names. It is clear enough that these are function (or method) names, and the parens suggest that they can be called without params, which is often not true. I don’t think there is a strong consensus about this for docstrings, so in this case I would preserve the existing style. (CPython does not value style uniformity over other things.)

ctrl-z-9000-times · 2022-01-07T15:56:12Z

I think what I'd like to do with this PR is to split it further into many many more PRs.
That way when we debate the wording of one sentence, it does not block changes to other sentences.
Also, that way I can avoid having to open bugs report to justify each of these minor changes.

I would really appreciate if someone could comment on PR #30223 which unlike all of these changes is actually important.

the-knights-who-say-ni added the CLA signed label Dec 26, 2021

bedevere-bot added the awaiting review label Dec 26, 2021

ctrl-z-9000-times mentioned this pull request Dec 26, 2021

bpo-46071: Improve graphlib documentation & reverse edge direction. #30102

Closed

ctrl-z-9000-times changed the title ~~Improve graphlib's documentation.~~ graphlib documentation: general improvements Dec 26, 2021

AlexWaygood added docs Documentation in the Doc dir skip news labels Dec 26, 2021

bpo-46204: Improve graphlib's documentation.

785754b

ctrl-z-9000-times changed the title ~~graphlib documentation: general improvements~~ bpo-46204: graphlib documentation: general improvements Dec 30, 2021

merwok reviewed Dec 31, 2021

View reviewed changes

ctrl-z-9000-times closed this Jan 22, 2022



		If any cycle is detected, :exc:`CycleError` will be raised.
		Raises :exc:`CycleError` if any cycles are detected.

Uh oh!

Conversation

ctrl-z-9000-times commented Dec 26, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexWaygood commented Dec 28, 2021

Uh oh!

ctrl-z-9000-times commented Dec 30, 2021

Uh oh!

merwok left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctrl-z-9000-times commented Jan 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ctrl-z-9000-times commented Dec 26, 2021 •

edited by bedevere-bot

Loading