Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/pull_request_template.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ Provide a concise overview of the issue or rationale behind the proposed changes
#### Checklist
- [ ] This PR targets the `main` branch. <!-- Backports will be evaluated and done by mergers, when necessary. -->
- [ ] The commit message is written in past tense, mentions the ticket number, and ends with a period.
- [ ] I have checked the "Has patch" ticket flag in the Trac system.
- [ ] I have checked the "Has pull request" ticket flag in the Trac system.
Copy link
Contributor

Choose a reason for hiding this comment

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

As commented in the Trac PR I think we need a more generic term, such as "Has proposal".

- [ ] I have added or updated relevant tests.
- [ ] I have added or updated relevant docs, including release notes if applicable.
- [ ] I have attached screenshots in both light and dark modes for any UI changes.
4 changes: 2 additions & 2 deletions .github/workflows/new_contributor_pr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ jobs:
pr-message: |
Hello! Thank you for your contribution 💪

As it's your first contribution be sure to check out the [patch review checklist](https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/submitting-patches/#patch-review-checklist).
As it's your first contribution be sure to check out the [contribution checklist](https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/submitting-patches/#patch-review-checklist).

If you're fixing a ticket [from Trac](https://code.djangoproject.com/) make sure to set the _"Has patch"_ flag and include a link to this PR in the ticket!
If you're fixing a ticket [from Trac](https://code.djangoproject.com/) make sure to set the _"Has pull request"_ flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the [Django forum](https://forum.djangoproject.com/c/internals/5).

Expand Down
10 changes: 5 additions & 5 deletions CONTRIBUTING.rst
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ As an open source project, Django welcomes contributions of many forms.

Examples of contributions include:

* Code patches
* Code contributions
* Documentation improvements
* Bug reports and patch reviews
* Bug reports and code reviews

Extensive contribution guidelines are available in the repository at
``docs/internals/contributing/``, or online at:
Expand All @@ -21,9 +21,9 @@ Trac tickets will be closed!** `Please file a ticket`__ to suggest changes.
__ https://code.djangoproject.com/newticket

Django uses Trac to keep track of bugs, feature requests, and associated
patches because GitHub doesn't provide adequate tooling for its community.
Patches can be submitted as pull requests, but if you don't file a ticket,
it's unlikely that we'll notice your contribution.
code contributions. Code contributions can be submitted as pull requests,
but if you don't file a ticket, it's unlikely that we'll notice your
contribution.
Comment on lines +24 to +26
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
code contributions. Code contributions can be submitted as pull requests,
but if you don't file a ticket, it's unlikely that we'll notice your
contribution.
code contributions, which can be submitted as pull requests in GitHub.
If you don't file a ticket, it's unlikely that we'll notice your contribution.


Code of Conduct
===============
Expand Down
2 changes: 1 addition & 1 deletion docs/internals/_images/triage_process.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 1 addition & 1 deletion docs/internals/contributing/bugs-and-features.txt
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ are a few additional guidelines to follow:
capturing a *brief* screencast. If your software permits it, capture only
the relevant area of the screen.

* If you're offering a patch that changes the look or behavior of Django's
* If you're proposing a change that impacts the look or behavior of Django's
UI, you **must** attach before *and* after screenshots/screencasts.
Tickets lacking these are difficult for triagers to assess quickly.

Expand Down
13 changes: 6 additions & 7 deletions docs/internals/contributing/committing-code.txt
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ contribute code to Django, look at :doc:`writing-code/working-with-git` instead.
Handling pull requests
======================

Since Django is hosted on GitHub, patches are provided in the form of pull
requests.
Django is hosted on GitHub and uses pull requests (PRs) for contributions.

When committing a pull request, make sure each individual commit matches the
commit guidelines described below. Contributors are expected to provide the
Expand Down Expand Up @@ -74,10 +73,10 @@ line.
When rewriting the commit history of a pull request, the goal is to make
Django's commit history as usable as possible:

* If a patch contains back-and-forth commits, then rewrite those into one.
For example, if a commit adds some code and a second commit fixes stylistic
issues introduced in the first commit, those commits should be squashed
before merging.
* If a contribution contains back-and-forth commits, then rewrite those into
one. For example, if a commit adds some code and a second commit fixes
stylistic issues introduced in the first commit, those commits should be
squashed before merging.

* Separate changes to different commits by logical grouping: if you do a
stylistic cleanup at the same time as you do other changes to a file,
Expand All @@ -89,7 +88,7 @@ Django's commit history as usable as possible:
* Tests should pass and docs should build after each commit. Neither the
tests nor the docs should emit warnings.

* Trivial and small patches usually are best done in one commit. Medium to
* Trivial and small changes usually are best done in one commit. Medium to
large work may be split into multiple commits if it makes sense.

Practicality beats purity, so it is up to each merger to decide how much
Expand Down
4 changes: 2 additions & 2 deletions docs/internals/contributing/localizing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@ must:
Django Git ``main`` branch, as for any code change.

* Open a ticket in Django's ticket system, set its ``Component`` field to
``Translations``, set the "has patch" flag, and include the link to the pull
request.
``Translations``, set the "has pull request" flag, and include the link to
the pull request.

.. _Transifex: https://www.transifex.com/
.. _Django project page: https://app.transifex.com/django/django/
Expand Down
57 changes: 29 additions & 28 deletions docs/internals/contributing/new-contributors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -27,39 +27,39 @@ Triage tickets
If an `unreviewed ticket`_ reports a bug, try and reproduce it. If you can
reproduce it and it seems valid, make a note that you confirmed the bug and
accept the ticket. Make sure the ticket is filed under the correct component
area. Consider writing a patch that adds a test for the bug's behavior, even if
you don't fix the bug itself. See more at :ref:`how-can-i-help-with-triaging`
area. Consider writing a test for the bug's behavior, even if you don't fix the
bug itself. See more at :ref:`how-can-i-help-with-triaging`

Review patches of accepted tickets
----------------------------------
Review pull requests of accepted tickets
Copy link
Contributor

Choose a reason for hiding this comment

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

In this section, I think "patch" is the correct term. That's what we review after all :-)

Also by not changing the header, we don't change how the anchor looks and links like.

----------------------------------------

This will help you build familiarity with the codebase and processes. Mark the
appropriate flags if a patch needs docs or tests. Look through the changes a
patch makes, and keep an eye out for syntax that is incompatible with older but
still supported versions of Python. :doc:`Run the tests
appropriate flags if a pull request needs docs or tests. Look through the
files changed in the PR, and keep an eye out for syntax that is incompatible
with older but still supported versions of Python. :doc:`Run the tests
Comment on lines +37 to +39
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
appropriate flags if a pull request needs docs or tests. Look through the
files changed in the PR, and keep an eye out for syntax that is incompatible
with older but still supported versions of Python. :doc:`Run the tests
appropriate flags if a change needs docs or tests. Look through the changes a
contribution makes, and keep an eye out for syntax that is incompatible with
older but still supported versions of Python. :doc:`Run the tests

</internals/contributing/writing-code/unit-tests>` and make sure they pass.
Where possible and relevant, try them out on a database other than SQLite.
Leave comments and feedback!

Keep old patches up-to-date
---------------------------
Keep old pull requests up-to-date
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, I think "patch" is the correct term here, and also as before by not changing the header, we don't change how the anchor looks and links like.

---------------------------------

Oftentimes the codebase will change between a patch being submitted and the
time it gets reviewed. Make sure it still applies cleanly and functions as
expected. Updating a patch is both useful and important! See more on
:doc:`writing-code/submitting-patches`.
Oftentimes the codebase will change between a pull request being submitted and
the time it gets reviewed. Make sure there are no merge conflicts and that the
code still functions as expected. Updating a pull request is both useful and
important! See more on :doc:`writing-code/submitting-patches`.
Comment on lines +47 to +50
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Oftentimes the codebase will change between a pull request being submitted and
the time it gets reviewed. Make sure there are no merge conflicts and that the
code still functions as expected. Updating a pull request is both useful and
important! See more on :doc:`writing-code/submitting-patches`.
Oftentimes the codebase will evolve between a change being submitted and the
time it gets reviewed. Make sure the diff still applies cleanly and functions
as expected. Updating a pull request is both useful and important! See more on
:doc:`writing-code/submitting-patches`.


Write some documentation
------------------------

Django's documentation is great but it can always be improved. Did you find a
typo? Do you think that something should be clarified? Go ahead and suggest a
documentation patch! See also the guide on :doc:`writing-documentation`.
documentation change! See also the guide on :doc:`writing-documentation`.

.. note::

The `reports page`_ contains links to many useful Trac queries, including
several that are useful for triaging tickets and reviewing patches as
several that are useful for triaging tickets and reviewing pull requests as
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
several that are useful for triaging tickets and reviewing pull requests as
several that are useful for triaging tickets and reviewing changes as

suggested above.

.. _reports page: https://code.djangoproject.com/wiki/Reports
Expand Down Expand Up @@ -118,7 +118,7 @@ Be bold! Leave feedback!
------------------------

Sometimes it can be scary to put your opinion out to the world and say "this
ticket is correct" or "this patch needs work", but it's the only way the
ticket is correct" or "this pull request needs work", but it's the only way the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ticket is correct" or "this pull request needs work", but it's the only way the
ticket is correct" or "this solution needs work", but it's the only way the

project moves forward. The contributions of the broad Django community
ultimately have a much greater impact than that of any one person. We can't do
it without **you**!
Expand All @@ -141,25 +141,26 @@ wayside ends up doing more harm than good.
Be rigorous
-----------

When we say ":pep:`8`, and must have docs and tests", we mean it. If a patch
doesn't have docs and tests, there had better be a good reason. Arguments like
"I couldn't find any existing tests of this feature" don't carry much weight.
While it may be true, that means you have the extra-important job of writing
the very first tests for that feature, not that you get a pass from writing
tests altogether.
When we say ":pep:`8`, and must have docs and tests", we mean it. If a pull
request doesn't have docs and tests, there had better be a good reason.
Arguments like "I couldn't find any existing tests of this feature" don't carry
much weight. While it may be true, that means you have the extra-important job
of writing the very first tests for that feature, not that you get a pass from
writing tests altogether.
Comment on lines +144 to +149
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When we say ":pep:`8`, and must have docs and tests", we mean it. If a pull
request doesn't have docs and tests, there had better be a good reason.
Arguments like "I couldn't find any existing tests of this feature" don't carry
much weight. While it may be true, that means you have the extra-important job
of writing the very first tests for that feature, not that you get a pass from
writing tests altogether.
When we say ":pep:`8`, and must have docs and tests", we mean it. If a change
doesn't have docs and tests, there had better be a good reason. Arguments like
"I couldn't find any existing tests of this feature" don't carry much weight.
While it may be true, that means you have the extra-important job of writing
the very first tests for that feature, not that you get a pass from writing
tests altogether.


Be patient
----------

It's not always easy for your ticket or your patch to be reviewed quickly. This
isn't personal. There are a lot of tickets and pull requests to get through.
It's not always easy for your ticket or pull request to be reviewed quickly.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
It's not always easy for your ticket or pull request to be reviewed quickly.
It's not always easy for your ticket or contribution to be reviewed quickly.

This isn't personal. There are a lot of tickets and pull requests to get
through.

Keeping your patch up to date is important. Review the ticket on Trac to ensure
that the *Needs tests*, *Needs documentation*, and *Patch needs improvement*
flags are unchecked once you've addressed all review comments.
Keeping your pull request up to date is important. Review the ticket on Trac to
Copy link
Contributor

Choose a reason for hiding this comment

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

I think patch is the accurate term here, which is really what needs to be kept updated:

Suggested change
Keeping your pull request up to date is important. Review the ticket on Trac to
Keeping your patch up to date is important. Review the ticket on Trac to ensure

ensure that the *Needs tests*, *Needs documentation*, and *Pull request needs
improvement* flags are unchecked once you've addressed all review comments.

Remember that Django has an eight-month release cycle, so there's plenty of
time for your patch to be reviewed.
time for your contribution to be reviewed.

Finally, a well-timed reminder can help. See :ref:`contributing code FAQ
<new-contributors-faq>` for ideas here.
Expand Down
86 changes: 43 additions & 43 deletions docs/internals/contributing/triaging-tickets.txt
Original file line number Diff line number Diff line change
Expand Up @@ -70,17 +70,17 @@ By way of example, here we see the lifecycle of an average ticket:
incorrect implementation).

* Bob reviews the pull request, marks the ticket as "Accepted", "needs tests",
and "patch needs improvement", and leaves a comment telling Alice how the
patch could be improved.
and "pull request needs improvement", and leaves a comment telling Alice how
the pull request could be improved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
the pull request could be improved.
the solution could be improved.


* Alice updates the pull request, adding tests (but not changing the
implementation). She removes the two flags.

* Charlie reviews the pull request and resets the "patch needs improvement"
flag with another comment about improving the implementation.
* Charlie reviews the pull request and resets the "pull request needs
improvement" flag with another comment about improving the implementation.

* Alice updates the pull request, fixing the implementation. She removes the
"patch needs improvement" flag.
"pull request needs improvement" flag.

* Daisy reviews the pull request and marks the ticket as "Ready for checkin".

Expand Down Expand Up @@ -114,39 +114,40 @@ Beyond that there are several considerations:

* **Accepted + No Flags**

The ticket is valid, but no one has submitted a patch for it yet. Often this
means you could safely start writing a fix for it. This is generally more
true for the case of accepted bugs than accepted features. A ticket for a bug
that has been accepted means that the issue has been verified by at least one
triager as a legitimate bug - and should probably be fixed if possible. An
accepted new feature may only mean that one triager thought the feature would
be good to have, but this alone does not represent a consensus view or imply
with any certainty that a patch will be accepted for that feature. Seek more
feedback before writing an extensive contribution if you are in doubt.
The ticket is valid, but no one has submitted a pull request for it yet.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The ticket is valid, but no one has submitted a pull request for it yet.
The ticket is valid, but no one has submitted a solution for it yet.

Often this means you could safely start writing a fix for it. This is
generally more true for the case of accepted bugs than accepted features. A
ticket for a bug that has been accepted means that the issue has been
verified by at least one triager as a legitimate bug - and should probably be
fixed if possible. An accepted new feature may only mean that one triager
thought the feature would be good to have, but this alone does not represent
a consensus view or imply with any certainty that a pull request will be
Copy link
Contributor

Choose a reason for hiding this comment

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

again I think patch is the correct term here:

Suggested change
a consensus view or imply with any certainty that a pull request will be
a consensus view or imply with any certainty that a patch will be

accepted for that feature. Seek more feedback before writing an extensive
contribution if you are in doubt.

* **Accepted + Has Patch**
* **Accepted + Has Pull Request**

The ticket is waiting for people to review the supplied solution. This means
downloading the patch and trying it out, verifying that it contains tests
and docs, running the test suite with the included patch, and leaving
feedback on the ticket.
checking out the pull request and trying it out, verifying that it contains
tests and docs, checking that all the automated tests on the pull request are
passing, and leaving feedback.
Comment on lines +131 to +133
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually checking out a pull request is not an easy task, specially when the PR is a branch in another repo (which is what we advice, make your fork and propose a PR from there). It's easier to get the patch (for example via the .patch URL) and apply it to your local repo than checking out someone else's branch. With this in mind, I would propose:

Suggested change
checking out the pull request and trying it out, verifying that it contains
tests and docs, checking that all the automated tests on the pull request are
passing, and leaving feedback.
getting the changes and trying them out, verifying that they contain tests
and docs, running the test suite with the included changes, and leaving
feedback on the ticket.


* **Accepted + Has Patch + Needs ...**
* **Accepted + Has Pull Request + Needs ...**

This means the ticket has been reviewed, and has been found to need further
work. "Needs tests" and "Needs documentation" are self-explanatory. "Patch
needs improvement" will generally be accompanied by a comment on the ticket
explaining what is needed to improve the code.
work. "Needs tests" and "Needs documentation" are self-explanatory. "Pull
request needs improvement" will generally be accompanied by a comment on the
ticket explaining what is needed to improve the code.

Ready For Checkin
-----------------

The ticket was reviewed by any member of the community other than the person
who supplied the patch and found to meet all the requirements for a
who submitted the pull request and found to meet all the requirements for a
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
who submitted the pull request and found to meet all the requirements for a
who submitted the changes and found to meet all the requirements for a

commit-ready contribution. A :ref:`merger <mergers-team>` now needs to give
a final review prior to being committed.

There are a lot of pull requests. It can take a while for your patch to get
There are a lot of pull requests. It can take a while for yours to get
reviewed. See the :ref:`contributing code FAQ<new-contributors-faq>` for some
ideas here.

Expand All @@ -158,44 +159,43 @@ high-level ideas or long-term feature requests.

These tickets are uncommon and overall less useful since they don't describe
concrete actionable issues. They are enhancement requests that we might
consider adding someday to the framework if an excellent patch is submitted.
They are not a high priority.
consider adding someday to the framework if an excellent contribution is
submitted. They are not a high priority.

Other triage attributes
=======================

A number of flags, appearing as checkboxes in Trac, can be set on a ticket:

Has patch
---------
Has pull request
----------------

This means the ticket has an associated solution. These will be reviewed to
ensure they adhere to the :doc:`documented guidelines
<writing-code/submitting-patches>`.

The following three fields (Needs documentation, Needs tests,
Patch needs improvement) apply only if a patch has been supplied.
Pull request needs improvement) apply only if a pull request has been supplied.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Pull request needs improvement) apply only if a pull request has been supplied.
Pull request needs improvement) apply only if a solution has been supplied.


Needs documentation
-------------------

This flag is used for tickets with patches that need associated
documentation. Complete documentation of features is a prerequisite
before we can check them into the codebase.
This flag is used for tickets with pull requests that need associated
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This flag is used for tickets with pull requests that need associated
This flag is used for tickets with solutions that need associated

documentation. Complete documentation of features is a prerequisite before we
can check them into the codebase.
Comment on lines +184 to +185
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to make this change I think, to reduce the diff:

Suggested change
documentation. Complete documentation of features is a prerequisite before we
can check them into the codebase.
documentation. Complete documentation of features is a prerequisite
before we can check them into the codebase.


Needs tests
-----------

This flags the patch as needing associated unit tests. Again, this
is a required part of a valid contribution.
This flags the pull request as needing associated unit tests. Again, this is a
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This flags the pull request as needing associated unit tests. Again, this is a
This flags the solution as needing associated unit tests. Again, this

required part of a valid contribution.

Patch needs improvement
-----------------------
Pull request needs improvement
------------------------------

This flag means that although the ticket *has* a solution, it's not quite
ready for checkin. This could mean the patch no longer applies
cleanly, there is a flaw in the implementation, or that the code
doesn't meet our standards.
ready for checkin. This could mean the pull request has merge conflicts, there
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not make edits here, I think patch is the most correct term.

is a flaw in the implementation, or that the code doesn't meet our standards.

Easy pickings
-------------
Expand Down Expand Up @@ -296,7 +296,7 @@ If you do close a ticket, you should always make sure of the following:
A ticket can be resolved in a number of ways:

* fixed
Used once a patch has been rolled into Django and the issue is fixed.
Used once a commit has been merged and the issue is fixed.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use solution since a ticket may be fixed by multiple commits:

Suggested change
Used once a commit has been merged and the issue is fixed.
Used once a solution has been merged and the issue is fixed.


* invalid
Used if the ticket is found to be incorrect. This means that the
Expand Down Expand Up @@ -356,7 +356,7 @@ Then, you can help out by:
sparse to be actionable, or when they're feature requests requiring a
discussion on the `Django Forum`_ or |django-developers|.

* Correcting the "Needs tests", "Needs documentation", or "Has patch"
* Correcting the "Needs tests", "Needs documentation", or "Has pull request"
flags for tickets where they are incorrectly set.

* Setting the "`Easy pickings`_" flag for tickets that are small and
Expand All @@ -377,7 +377,7 @@ Then, you can help out by:
* Verify if solutions submitted by others are correct. If they are correct
and also contain appropriate documentation and tests then move them to the
"Ready for Checkin" stage. If they are not correct then leave a comment to
explain why and set the corresponding flags ("Patch needs improvement",
explain why and set the corresponding flags ("Pull request needs improvement",
"Needs tests" etc.).

.. note::
Expand All @@ -396,7 +396,7 @@ the ticket database:
* Please **don't** promote your own tickets to "Ready for checkin". You
may mark other people's tickets that you've reviewed as "Ready for
checkin", but you should get at minimum one other community member to
review a patch that you submit.
review a pull request that you submit.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
review a pull request that you submit.
review a solution that you submit.


* Please **don't** reverse a decision without posting a message to the
`Django Forum`_ or |django-developers| to find consensus.
Expand Down
Loading