Skip to content

Add the labels flowchart to the README. #500

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

Merged
merged 4 commits into from
Aug 8, 2022
Merged

Conversation

ezio-melotti
Copy link
Member

This PR adds a flowchart that describe the PR state machine to the README.

Related issues:

The flowchart is described in

# The following is the state machine for the flow of a PR
# (written as a DOT file; you can use Graphviz or
# http://www.webgraphviz.com/ to view the graph):
"""
digraph "PR stages" {
/* Colours represent who can make a state change or who is currently
blocking the PR from moving forward.
Blue: Anyone
Orange: PR creator
Green: Core developer
Purple: Triager Actions
*/
"New PR" [color=orange]
"Awaiting review" [shape=box, color=blue]
"Awaiting core review" [shape=box, color=green]
"Awaiting changes" [shape=box, color=orange]
"Awaiting change review" [shape=box, color=green]
"Awaiting merge" [shape=box, color=green]
"New PR" -> "Awaiting review" [label="New PR by a contributor", color=orange]
"Awaiting review" -> "Awaiting core review" [label="New review by another contributor", color=blue]
"Awaiting core review" -> "Awaiting core review" [label="New review by another contributor", color=blue]
"Awaiting core review" -> "Awaiting changes" [label="New core review requests changes", color=green]
"Awaiting changes" -> "Awaiting change review" [label="Changes are done by contributor\nBedevere requests review from core-dev", color=orange]
"Awaiting change review" -> "Awaiting changes" [label="New core review requests changes", color=green]
"Awaiting change review" -> "Awaiting merge" [label="New core review approves", color=green]
"Awaiting review" -> "Awaiting merge" [label="New core review approves", color=green]
"Awaiting review" -> "Awaiting changes" [label="New core review requests changes", color=green]
"Awaiting core review" -> "Awaiting merge" [label="New core review approves", color=green]
"New PR" -> "Awaiting core review" [label="New PR by core devs", color=green]
}
"""

which renders to

image

For the implementation I used mermaid, which is supported natively by GitHub-flavored Markdown. The new flowchart renders to:

flowchart TD
    A([New PR]):::creator
    A -- by contributor --> B[Awaiting review]:::anyone
    A -- by core dev --> C[Awaiting core review]:::coredev
    B & C -- new review by\nanother contributor --> C
    C & B & E  -- new core review\nrequests changes --> D[Awaiting changes]:::creator
    D -- changes by contributor --> E[Awaiting change review]:::coredev
    C & E & B -- new core review\napproves ---> F[Awaiting merge]:::coredev
classDef creator stroke:#CC0;
classDef anyone stroke:#00C;
classDef coredev stroke:#0C0;
linkStyle 0,1,7 stroke:#CC0,color:auto;
linkStyle 2,3 stroke:#00C,color:auto;
linkStyle 4,5,6,8,9,10 stroke:#0C0,color:auto;
Loading
Click to see a screenshot of the flowchart (using the dark theme)

image

@codecov
Copy link

codecov bot commented Aug 7, 2022

Codecov Report

Merging #500 (d85784a) into main (428290e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #500   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           18        18           
  Lines         1829      1828    -1     
  Branches       223       223           
=========================================
- Hits          1829      1828    -1     
Flag Coverage Δ
Python_3.10 100.00% <ø> (ø)
Python_3.11-dev 100.00% <ø> (ø)
Python_3.8 100.00% <ø> (ø)
Python_3.9 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
bedevere/stage.py 100.00% <ø> (ø)

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Should we remove the digraph from comments in stage.py and refer to this one?

@DanielNoord
Copy link
Contributor

Could this also close #184?

@ezio-melotti ezio-melotti mentioned this pull request Aug 7, 2022
@ezio-melotti
Copy link
Member Author

Should we remove the digraph from comments in stage.py and refer to this one?

I was thinking about that 🤔
In the new graph I tried to include everything that was included in the old one, with the exception of the CSS class for triagers (since it's unused). I don't think we are losing any information by deleting it, and I can always leave a comment mentioning it in case someone needs it.

@ezio-melotti
Copy link
Member Author

I removed the old graph and added a couple of comments.

@ezio-melotti ezio-melotti linked an issue Aug 7, 2022 that may be closed by this pull request
# The graph replaces a previous version (written as
# a DOT file) that was included here.
#
# Changes to this file should be reflected in the README.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this line necessary?

I'm not even sure the comments are necessary. They are probably relevant now but in 6/12 months time they will probably just become dead weight. I would be okay with removing them as well, but I'll leave that decision to you!

Copy link
Member Author

Choose a reason for hiding this comment

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

The one about updating the README is important to ensure the graph in the README keeps matching the code (and in turn the comment in the README ensures the devguide is updated -- assuming people see the comments).

The one about the previous version is not particularly important, but without it someone might end up wasting time looking for that graph they once saw in the code. Having a couple of extra lines for this comment is not a big cost.

@ezio-melotti ezio-melotti merged commit 793bcca into main Aug 8, 2022
@ezio-melotti ezio-melotti deleted the labels-flowchart branch August 8, 2022 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Improve state graph
3 participants