Skip to content
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

bpo-38101: Update devguide triaging keywords #570

Merged
merged 11 commits into from Feb 23, 2020

Conversation

@idomic
Copy link
Contributor

@idomic idomic commented Feb 2, 2020

Added 3 new keywords: pep 3121, security_issue, easy (C)

@idomic
Copy link
Contributor Author

@idomic idomic commented Feb 2, 2020

@taleinat I wasn't sure how to find a reviewer for this/the reviewer in the issue from Sept.

@taleinat taleinat changed the title bpo-38101 Updated devguide triaging keywords. bpo-38101: Updated devguide triaging keywords. Feb 3, 2020
@taleinat taleinat changed the title bpo-38101: Updated devguide triaging keywords. bpo-38101: Update devguide triaging keywords Feb 3, 2020
Copy link
Contributor

@taleinat taleinat left a comment

Hey @idomic, thanks for the PR!

It needs a bit more work but definitely in the right direction.

@@ -375,6 +375,13 @@ Various informational flags about the issue. Multiple values are possible.
| easy | Fixing the issue should not take longer than a day for |
| | someone new to contributing to Python to solve. |
+---------------+------------------------------------------------------------+
| easy (C) | Fixing the issue should not take longer than a day for |
| | someone new contributing to Python, focused on C/C++. |

This comment has been minimized.

@taleinat

taleinat Feb 3, 2020
Contributor

We just use C, so the mention of C++ here is out of place.

This comment has been minimized.

@idomic

idomic Feb 8, 2020
Author Contributor

Done.

triaging.rst Outdated Show resolved Hide resolved
+---------------+------------------------------------------------------------+
| security_issue| The issue would fit as, or is related as a security issue. |
+---------------+------------------------------------------------------------+
| PEP 3121 | The issue fit as, or is related to the PEP 3123 module |

This comment has been minimized.

@taleinat

taleinat Feb 3, 2020
Contributor

The PEP number here is off, it should be 3121. Also, a few words about the subject of the PEP, and a link to it, would be very helpful.

@idomic
Copy link
Contributor Author

@idomic idomic commented Feb 12, 2020

I have made the requested changes; please review again

Copy link
Contributor

@taleinat taleinat left a comment

Looking good, almost there! One more comment.

| PEP 3121 | The issue fit as, or is related to the PEP 3121 module |
| | Which is the Extension Module Initialization and |
| | Finalization, For More information: |
| | https://www.python.org/dev/peps/pep-3121/ |
Comment on lines 385 to 388

This comment has been minimized.

@taleinat

taleinat Feb 12, 2020
Contributor

The wording and capitalization here are a bit clumsy. My suggestion:

"The issue is related to `PEP 3121`_: Extension Module Initialization and Finalization."

(The above assumes adding the link for PEP 3121 below it.)

This comment has been minimized.

@idomic

idomic Feb 13, 2020
Author Contributor

I have made the requested changes; please review again

@idomic
Copy link
Contributor Author

@idomic idomic commented Feb 13, 2020

I have made the requested changes; please review again

Copy link
Contributor

@taleinat taleinat left a comment

Looking good @idomic, I've just a couple more small comments.

| easy (C) | Fixing the issue should not take longer than a day for |
| | someone new contributing to Python, focused on C. |
+---------------+------------------------------------------------------------+
| security_issue| The issue would fit as, or is related as a security issue. |

This comment has been minimized.

@taleinat

taleinat Feb 13, 2020
Contributor

Suggested change
| security_issue| The issue would fit as, or is related as a security issue. |
| security_issue| This is a security issue or is related to one. |
| | someone new contributing to Python, focused on C. |
+---------------+------------------------------------------------------------+
| security_issue| The issue would fit as, or is related as a security issue. |
| | The main difference from "security" is that this is a |

This comment has been minimized.

@taleinat

taleinat Feb 13, 2020
Contributor

"security" is out of context here, so maybe write something "The main difference from the "secutiry" issue type is..."

This comment has been minimized.

@idomic

idomic Feb 14, 2020
Author Contributor

Added both changes, I think also since there is no security keyword in the bug tracker, the main difference is that is a definite problem right? While the security type above it is more for reporting a possible issue.

@idomic
Copy link
Contributor Author

@idomic idomic commented Feb 14, 2020

I have made the requested changes; please review again

@idomic
Copy link
Contributor Author

@idomic idomic commented Feb 18, 2020

@taleinat I think the link to pep 3121 is failing the CI, any way to override it?

@taleinat
Copy link
Contributor

@taleinat taleinat commented Feb 18, 2020

@idomic, the CI is failing correctly!

The build fails because you haven't defined where the "PEP 3121" link should point to. It needs to be like this:

This is a paragraph that contains `a link`_.

.. _a link: https://domain.invalid/

For more info, check out similar links in this (or other ReST) documents, or the ReST Primer.

idomic added 5 commits Feb 22, 2020
@idomic
Copy link
Contributor Author

@idomic idomic commented Feb 23, 2020

Ok, after lots of commits (sorry haha), that's ready CI is passing.

Copy link
Contributor

@taleinat taleinat left a comment

LGTM

@taleinat taleinat merged commit d9cb45c into python:master Feb 23, 2020
6 checks passed
6 checks passed
Header rules No header rules processed
Details
Pages changed 2 new files uploaded
Details
Redirect rules No redirect rules processed
Details
Mixed content No mixed content detected
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@taleinat
Copy link
Contributor

@taleinat taleinat commented Feb 23, 2020

Thanks for taking this up and following through, @idomic!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.