Improve documentation for Graph component

Created on 24 February 2021, over 3 years ago
Updated 28 May 2024, about 2 months ago

Problem/Motivation

I've read this 4 times and I don't get it:

   * @param $graph
   *   A three dimensional associated array, with the first keys being the names
   *   of the vertices, these can be strings or numbers. The second key is
   *   'edges' and the third one are again vertices, each such key representing
   *   an edge. Values of array elements are copied over.
   *
   *   Example:
   *   @code
   *     $graph[1]['edges'][2] = 1;
   *     $graph[2]['edges'][3] = 1;
   *     $graph[2]['edges'][4] = 1;
   *     $graph[3]['edges'][4] = 1;
   *   @endcode
   *
   *   On return you will also have:
   *   @code
   *     $graph[1]['paths'][2] = 1;
   *     $graph[1]['paths'][3] = 1;
   *     $graph[2]['reverse_paths'][1] = 1;
   *     $graph[3]['reverse_paths'][1] = 1;
   *   @endcode
* A three dimensional associated array, with the first keys being the names
* of the vertices, these can be strings or numbers.

This sentence isn't grammatical.

The second key is
* 'edges' and the third one are again vertices

I wasn't sure if this was a typo and it should be quoted as 'vertices', a literal string. But looking at the example, it's not.

each such key representing
* an edge.

Does that mean that each key of the 'edges' array is the name of a vertex, that is, matches a key in the main array?

> Values of array elements are copied over

I have no idea what that means.

Copied over -- does that mean copied from one place to another? In which case, from where to where? Or does it mean overwritten?

Is it trying to say that the values don't mean anything? They are all 1 in the example code, so I suspect that might be the case.

> * On return you will also have:

Return? This is a constructor. It doesn't return anything. What does this mean?

Remaining tasks

  • Convert the latest patch to a merge request.
  • Update the docblock as per #16.
📌 Task
Status

Needs work

Version

11.0 🔥

Component
Documentation 

Last updated about 2 hours ago

No maintainer
Created by

🇬🇧United Kingdom joachim

Live updates comments and jobs are added and updated live.
  • Novice

    It would make a good project for someone who is new to the Drupal contribution process. It's preferred over Newbie.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇺🇸United States ksenzee Seattle area

    I think the docblock still needs more of a rewrite.

    > Return? This is a constructor. It doesn't return anything. What does this mean?

    My guess is that at some point this object was refactored, and the documentation didn't get a matching refactor. The constructor says it instantiates the "depth first search" object. It probably used to contain the code that's now in ::searchAndSort(), and when that got moved to its own method, the docblock didn't move along with it. So the constructor docblock should instead start out with "Instantiates the directed acyclic graph object."

    For the description under @param $graph, I'd say something more like

    A three-dimensional associative array, keyed by the names of the graph's vertices, which can be strings or numbers. Each vertex may have an 'edges' key, whose value is an array keyed by the names of the vertices connected to it; the values in this array can be simply TRUE or may contain other data.

    The example in the code block is good—vital even—but I think it should be preceded by a little diagram of the graph, like the one in the comments at \Drupal\Tests\Component\Graph\GraphTest::testDepthFirstSearch(). For the example given, the diagram would be

    1 --> 2 --> 3
          |     |
          |     v
          + --> 4
    

    or if we can use ASCII Extended in docblocks,

    1────>2────>3
          │     │
          │     ▼
          └───► 4
    

    The second @code block in the constructor—the one with 'paths' and 'reverse_paths' keys—should be moved to the ::searchAndSort() method, because it's describing the return value of ::searchandSort() if you pass it the graph described above.

  • 🇺🇸United States xjm

    Updating the remaining tasks section.

  • First commit to issue fork.
  • Status changed to Needs review about 2 months ago
  • 🇬🇧United Kingdom NexusNovaz

    Hi, I've made MR 8124 which is ready for review. I've added both the patch from #9 and the changes in #16.

    Thanks

  • Pipeline finished with Failed
    about 2 months ago
    Total: 168s
    #176475
  • Status changed to Needs work about 2 months ago
  • The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

  • Status changed to Needs review about 2 months ago
  • Pipeline finished with Failed
    about 2 months ago
    Total: 872s
    #177355
  • Status changed to Needs work about 2 months ago
  • 🇺🇸United States ksenzee Seattle area

    Thanks for working on this! It looks like the diagram needs an update—the code and the diagram don't currently match. I think lines 28-30 need to be indented, so that 2 and 3 lead to 4, not 1 and 2 leading to 4.

  • Status changed to Needs review about 2 months ago
  • 🇬🇧United Kingdom NexusNovaz

    Nice spot @ksenzee!

    Made that adjustment to the MR and should be ready for review!

  • Pipeline finished with Failed
    about 2 months ago
    Total: 899s
    #178346
  • Status changed to RTBC about 2 months ago
  • 🇺🇸United States smustgrave

    Failure is unrelated.

    Appears feedback has been addressed.

  • Status changed to Needs work about 2 months ago
  • 🇳🇿New Zealand quietone New Zealand

    @NexusNovaz, thank you for working on this issue! And for you prompt replies to feedback.

    I have reviewed the MR and don't think that the example should be split into two parts, one in the constructor and one in the method searchAndSort. Instead, let's use the example of migrate process plugins where the examples are in the Class doc bloc. See MigrationLookup for one example.

Production build 0.69.0 2024