Skip to content

Conversation

@aeros
Copy link
Contributor

@aeros aeros commented Aug 24, 2019

Based on the conversation in #527, I added a note after the "Responsibilities include" list which specifies that triagers should only close PRs that could be considered spam, but they can suggest for other PRs to be closed. For anything potentially valid (including recently active and older PRs), a core developer should make the final decision.

Copy link
Member

@Mariatta Mariatta left a comment

Choose a reason for hiding this comment

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

  • Remove line 31 (- Closing PRs and issues)
  • triagers should not close any PRs. Only core devs should do this.
  • There is now a "stale" label in CPython repo. Triagers can add this label to indicate that the PR is inactive/stale and potentially can be closed by core dev.

@Mariatta
Copy link
Member

Sorry, but I need a break from open source and about to be OOOS, I won't be able to look into this for a while. Can other core devs look into this PR?

@brettcannon brettcannon self-requested a review August 27, 2019 18:57
@aeros
Copy link
Contributor Author

aeros commented Aug 29, 2019

Sorry, but I need a break from open source and about to be OOOS, I won't be able to look into this for a while. Can other core devs look into this PR?

No problem, thanks for all the help with getting the Triage team set up @Mariatta!

@mariatta-bot
Copy link

🤖 Mariatta was mentioned, but she's out of open source until end of September 2019. Hopefully someone else can look into this in the meantime.

@aeros
Copy link
Contributor Author

aeros commented Aug 29, 2019

Mariatta:

Remove line 31 (- Closing PRs and issues)
triagers should not close any PRs. Only core devs should do this.
There is now a "stale" label in CPython repo. Triagers can add this label to indicate that the PR is inactive/stale and potentially can be closed by core dev.

I applied all of the above suggestions to the PR and updated the labels section to include the new stale label. I also made a brief mention in the section for the invalid label to indicate that it can be used by triagers to suggest that a PR should be closed.

Based on my understanding of @brettcannon's earlier description, a stale PR is one that is either no longer relevant, or has not received a response from the author in a significant period of time. If possible, I think we should aim to more specifically define what would potentially qualify as a "significant period of time", but it might be somewhat circumstantial and better left open to interpretation.

Based on feedback from Ammar Akar, clarify closing guidelines to only restrict the closing of PRs, not bpo issues.
Also, add a reference label for the GitHub PR labels section
@aeros
Copy link
Contributor Author

aeros commented Aug 31, 2019

Since the note has grown significantly in size, I think it might be better in terms to formatting to remove the note directive:

image

Also, I hadn't initially intended for the note to be on the same indentation level as the bulleted list. The netlify preview is quite useful, I'm looking forward to having it for CPython documentation changes.

@aeros
Copy link
Contributor Author

aeros commented Aug 31, 2019

Since @eamanu added a section for the stale label in his PR (#531), I'll remove it from this one.

@eamanu eamanu mentioned this pull request Aug 31, 2019
Copy link
Collaborator

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Thanks @aeros167. I reworded the triager/core dev responsibilities to be more positive.

Also, I recommend removing the competition sentence.

@aeros
Copy link
Contributor Author

aeros commented Sep 2, 2019

Thanks for the in-depth review @willingc. If any of my responses within the next week are delayed, I've been intermittently occupied with hurricane prep and may be without power for a few days depending on the trajectory.

Thanks @aeros167. I reworded the triager/core dev responsibilities to be more positive.

I might have been basing the wording too much on the discussion from the ML and issue. More positivity is always a good thing. (:

Also, I recommend removing the competition sentence.

I'm not clear why the competitions are mentioned. I would delete that sentence since GitHub will recognize the contribution if accepted.

That was based on what Mariatta said in a discuss topic regarding the GitHub labels:

invalid: This can be automatically added by bedevere when people try to merge maintenance branch to the master branch. It can be manually added by us too if we deem it invalid. The effect is, the PR won;t count towards user’s contribution during hacktoberfest.

I'm perfectly okay with removing it though.

Edit: Normally I'd @mention Mariatta, but it looks like she's currently taking a well deserved break from open source for a couple of months.

Co-Authored-By: Carol Willing <carolcode@willingconsulting.com>
@aeros
Copy link
Contributor Author

aeros commented Sep 2, 2019

I'll fix the number of words per line after all of the suggestions are finished so that I can do it all at once instead of across multiple commits.

@willingc
Copy link
Collaborator

willingc commented Sep 2, 2019

Stay safe @aeros167 during the storm.

@aeros
Copy link
Contributor Author

aeros commented Sep 3, 2019

Stay safe @aeros167 during the storm.

Thanks. It seems to be going rather slowly at the moment and the predicted path keeps changing. Fortunately though, it seems be a lot less likely to make direct landfall with the latest models, but we're still likely going to have some outages.

Co-Authored-By: Carol Willing <carolcode@willingconsulting.com>
Copy link
Contributor

@eamanu eamanu left a comment

Choose a reason for hiding this comment

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

LGTM

@aeros
Copy link
Contributor Author

aeros commented Sep 7, 2019

@willingc I believe that I've addressed all of the recommended changes. Is there anything further that I should change before moving forward with this PR?

@brettcannon brettcannon merged commit 23fff35 into python:master Sep 11, 2019
@aeros
Copy link
Contributor Author

aeros commented Sep 11, 2019

Thanks @brettcannon!

AA-Turner pushed a commit to AA-Turner/devguide that referenced this pull request Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants