GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Update the triage/triaging.rst page. by ezio-melotti · Pull Request #914 · python/devguide

Conversation

@ezio-melotti

@ezio-melotti ezio-melotti commented Jul 12, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This PR updates the triage/triaging.rst page, which contained outdated references to bpo and HG's patch-based workflow.

See https://cpython-devguide--914.org.readthedocs.build/triage/triaging/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@ezio-melotti
Update the triage/triaging.rst page.
1b14292
@ezio-melotti ezio-melotti added the type-feature Additions; New content or section needed label Jul 12, 2022
@ezio-melotti ezio-melotti self-assigned this Jul 12, 2022
@ezio-melotti ezio-melotti linked an issue Jul 12, 2022 that may be closed by this pull request
Update "Checklist for Triaging" for GH Issues #850
Closed
@erlend-aasland

erlend-aasland commented Jul 12, 2022

Copy link
Copy Markdown
Contributor

This is a nice improvement. IMO, when issues are closed, we should also encourage leaving a message regarding why it was closed, for example "implemented in the following PRs: ", "superseded by ", etc.

AlexWaygood, arhadthedev, hugovk, and ezio-melotti reacted with thumbs up emoji
All reactions
  • 👍 4 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Jul 12, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Also, I made a review checklist for one of my PRs:

python/cpython#93823 (comment)

Perhaps a generalised version could be added to the devguide?

ezio-melotti and CAM-Gerlach reacted with heart emoji
All reactions
  • ❤️ 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ezio-melotti added 2 commits August 6, 2022 20:02
@ezio-melotti
Mention the triaged label + more tweaks.
e4f4a77
@ezio-melotti
Fix markup.
19db210
@erlend-aasland
erlend-aasland requested review from ambv and iritkatriel August 6, 2022 18:11
@ezio-melotti

ezio-melotti commented Aug 6, 2022

Copy link
Copy Markdown
Member Author

I added a mention of the triaged label and made a few other tweaks.

Perhaps a generalised version could be added to the devguide?

SGTM -- that's a nice list. I think it would be easier to add it in a separate PR though.
Reviewing PRs is currently documented in two places:

  • https://devguide.python.org/getting-started/pull-request-lifecycle/#reviewing
  • https://devguide.python.org/triage/triaging/#reviewing-patches

The former has been likely written from scratch after the HG->Git migration, whereas the latter formerly documented how to review patches attached to a bpo issue and then it has been adapted to GitHub. I think the best approach would be to enhance the pull-request-lifecycle with your list, and remove the duplicated info in triaging.

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Aug 6, 2022

Copy link
Copy Markdown
Contributor

I think the best approach would be to enhance the pull-request-lifecycle with your list, and remove the duplicated info in triaging.

SGTM! Thanks for making all these enhancements to our (documented) workflow.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland
erlend-aasland approved these changes Aug 6, 2022
View reviewed changes
@ezio-melotti
ezio-melotti requested a review from CAM-Gerlach October 7, 2022 05:09
@ezio-melotti ezio-melotti mentioned this pull request Oct 7, 2022
Improve PR review checklist #962
Open
@ezio-melotti

ezio-melotti commented Oct 7, 2022

Copy link
Copy Markdown
Member Author

SGTM -- that's a nice list. I think it would be easier to add it in a separate PR though.

I created #962 to track this, so that we can start merging this and keep improving the devguide iteratively (as Diataxis suggests).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ezio-melotti
Merge branch 'main' into update-triaging
87c3027
ezio-melotti
ezio-melotti commented Oct 7, 2022
View reviewed changes
Comment thread triage/triaging.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@ezio-melotti
Add a link to the triaged label.
e928c57
iritkatriel
iritkatriel reviewed Oct 7, 2022
View reviewed changes
Comment thread triage/triaging.rst Outdated
* You might also leave a brief comment about the proposed next action needed.
If there is a long message list, a summary can be very helpful.
* Finally, you can set the :gh-label:`triaged` label (unless you want other triagers
to take a look).

@iritkatriel iritkatriel Oct 7, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is so far the first and only documentation we have about the "triaged" label, so let me try to understand what it's for. The idea is that if a triager does not want other triagers to look at the issue, they put a triaged label?

Why would a triager not want other triagers to look at the issue?

Is it then offensive for another triager or core dev to change the labels on the issue (undermining a decision that another triager marked as 'final')?

This sounds to me like the opposite of how we work. It's the equivalent of a label that a no-committer can put on a PR to say "this has been reviewed. I do not want other non-commiters to review it". We don't have this label. We have the "awaiting core review" label which indicates that a non-committer has reviewed, but it does not dis-invite further reviews from non-committers.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@iritkatriel iritkatriel Oct 7, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I see now that if I hover over the label it says "The issue has been accepted as valid by a triager". Which is quite different from what is implied by this doc.

I don't think validity is something we expect triagers to decide about (when I proposed a "valid" label I tried to be clear that it is to be used by subject matter experts, not triagers). If you think that this is for triagers to decide, then this doc should explain how a triager is supposed to make that decision. If you don't think that, then the tooltip on the "triaged" label needs to change to whatever it is that this label means.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@ezio-melotti ezio-melotti Oct 7, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I don't remember the exact timeline, but I wrote that sentence on Aug 6 and I might have added the label description on GitHub later after more discussions.

However it seems there is still no clear consensus on how it should be used. Are you at the sprints? Perhaps it could be discussed there.

Regarding this specific PR, it is mostly about updating the checklist, not documenting the label itself. I can remove or change the sentence, and possibly re-add/update it again in next iteration. I also have another incoming PR (#930) where I can document it more extensively.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@terryjreedy terryjreedy Oct 7, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

What meaning did the Steering Council intend when they approved the new term?

I agree with deferring the Triaged sentence to get on with the improved list.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@ezio-melotti ezio-melotti Oct 7, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

See https://discuss.python.org/t/improve-communication-with-contributors-on-the-issue-tracker-and-prs-language-summit-follow-up/15812/30, in particular:

The triaged label would signify that the process is done (for now, at least) – and since the issue wasn’t closed, it’s “valid” and waiting for a core dev.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@iritkatriel iritkatriel Oct 7, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Are you at the sprints? Perhaps it could be discussed there.

I'm online, but I don't feel able to contribute on this matter beyond commenting on whether I think the documentation is consistent and clear.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ezio-melotti reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@encukou encukou Oct 10, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The idea is that if a triager does not want other triagers to look at the issue, they put a triaged label?

No, the idea is that if a triager thinks there is nothing more a triager can do, they mark the issue so others don't need to re-read it.

Is it then offensive for another triager or core dev to change the labels on the issue (undermining a decision that another triager marked as 'final')?

That would be like any other disagreement. It could be done in an offensive way, but hopefully won't.

this has been reviewed. I do not want other non-commiters to review it

I'd word it as “This has been reviewed. Other non-commiters are free to re-review it, but I believe their time would be better spent elsewhere.”

(I don't speak for the entire Steering Council here.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ezio-melotti and erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

@iritkatriel iritkatriel Oct 11, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The triaged label would signify that the process is done (for now, at least)

Does “for now, at least” mean that the triaged label will be removed by a bot after a certain amount of time? Or how else will it work long term?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
ezio-melotti
ezio-melotti commented Oct 7, 2022
View reviewed changes
Comment thread triage/triaging.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

CAM-Gerlach
CAM-Gerlach reviewed Oct 7, 2022
View reviewed changes

@CAM-Gerlach CAM-Gerlach left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Some feedback

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread triage/triaging.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread triage/triaging.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread triage/triaging.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread triage/triaging.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread triage/triaging.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread triage/triaging.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread triage/triaging.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread triage/triaging.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread triage/triaging.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread triage/triaging.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

ezio-melotti and others added 2 commits October 8, 2022 02:09
@ezio-melotti @CAM-Gerlach
Apply suggestions from code review
034b1a3
Co-authored-by: C.A.M. Gerlach <[email protected]>
@ezio-melotti
Address more review comments.
5bd2948
CAM-Gerlach
CAM-Gerlach reviewed Oct 8, 2022
View reviewed changes

@CAM-Gerlach CAM-Gerlach left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sorry, my review was a bit rushed to get it in before lunch and I ended up missing a few things the first go around. Otherwise, LGTM, thanks 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ezio-melotti reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread triage/triaging.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread triage/triaging.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread triage/triaging.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread triage/triaging.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@ezio-melotti @CAM-Gerlach
Apply suggestions from code review
f530bca
Co-authored-by: C.A.M. Gerlach <[email protected]>
@ezio-melotti
ezio-melotti merged commit 46b4a82 into python:main Oct 8, 2022
@ezio-melotti
ezio-melotti deleted the update-triaging branch October 8, 2022 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@encukou encukou encukou left review comments
@iritkatriel iritkatriel iritkatriel left review comments
@CAM-Gerlach CAM-Gerlach CAM-Gerlach left review comments
@terryjreedy terryjreedy terryjreedy left review comments
@erlend-aasland erlend-aasland erlend-aasland approved these changes
@ambv ambv Awaiting requested review from ambv

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@ezio-melotti ezio-melotti

Labels

type-feature Additions; New content or section needed

Projects

CPython labels
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Update "Checklist for Triaging" for GH Issues

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@ezio-melotti @erlend-aasland @encukou @iritkatriel @CAM-Gerlach @terryjreedy
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.