8000 Add missing command for running-test on contributing page by baidoosik · Pull Request #19068 · django/django · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add missing command for running-test on contributing page #19068

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

Open
wants to me 8000 rge 1 commit into
base: main
Choose a base branch
from

Conversation

baidoosik
Copy link
Contributor
@baidoosik baidoosik commented Jan 18, 2025

Trac ticket number

N/A

Branch description

The command for installing the Django package was missing in the documentation. This omission may cause confusion for people following the guide, potentially leading to issues when running tests. I encountered this issue myself and found the correct command in the tests/README.rst.

This PR adds the missing python -m pip install -e .. command to ensure that the test dependencies are properly installed before running the test suite.

Checklist

  • This PR targets the main branch.
  • 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 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.

@baidoosik baidoosik force-pushed the minor_doc_enhance branch 2 times, most recently from 62664c8 to c9a9d18 Compare January 18, 2025 15:41
@adamzap
Copy link
Member
adamzap commented Jan 18, 2025

That step is mentioned a bit further up in the document:

My sense is that the intention of the tutorial is for new contributors to follow it from the beginning, so I think we can assume that the command has already been run. Are you thinking otherwise?

@baidoosik
Copy link
Contributor Author
baidoosik commented Jan 19, 2025

I understand your perspective. I thought someone might misunderstand that the above process is not for the django itself, it is an order to use the django development version in another project. When I copied the document, I intended it to be executed as part of the process. That said, after hearing your explanation and revisiting the document, I think my concerns were a little too much. If most of people think like you, I will close the PR. Thanks for review!

@adamzap
Copy link
Member
adamzap commented Jan 19, 2025

Thanks for considering my perspective! We can wait to see what someone else with more authority says.

Copy link
Contributor
@nessita nessita left a comment

Choose a reason for hiding this comment

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

I agree with Adam that repeating here the -e install line feels redundant, but I also see the confusion that @baidoosik presents. I think we can make a small tweaks to this page so instead of running -e again, it says something like "Ensure that you have followed and completed the previous section".

Proposal:

Before running the test suite, make sure you've followed the steps in the previous section, and then enter the Django tests/ directory...

"previous section" should be a local reference to the section before this one.

What do you think?

@timgraham
Copy link
Member

I don't understand the confusion. Why did you skip the previous section? I don't understand what "When I copied the document, I intended it to be executed as part of the process." means.

I'd prefer not to start a precedent of adding "make sure you've followed the steps in the previous section" to tutorials. If readers believe they know enough to skip steps, I wouldn't worry about warning them.

@baidoosik
Copy link
Contributor Author
baidoosik commented Jan 21, 2025

@timgraham I didn't skip the previous section. In the previous section, I totally understood I need to install local Django packages with editable mode when I want to use local Django package. So, when I failed to execute $ python -m pip install -r requirements/py3.txt command, I immediately noticed that the package needed to be installed.
What I'm trying to say is, People who read and run 'Running Django's test suite for the first time.' are more likely to execute the command in the order in which it is written without knowing what dependency it has. I just want to make more kinder document for first contributor. So, I'd like to add explicitly that needs local django package for running test as @nessita suggestion.

If agree with my opinion, tell me. I will update code with @nessita's feedback.

@timgraham
Copy link
Member

I'm afraid I still don't understand how a reader ends up at "Running Django's test suite for the first time" while skipping part of the previous section. Isn't the purpose of the proposed clarification aimed at someone who did?

@baidoosik
Copy link
Contributor Author

@timgraham Let me clarify my point. Even if someone reads the previous section completely, first-time Django contributors might not connect that local Django installation is a prerequisite for running tests. While the previous section explains installing local Django in editable mode, it might not be immediately obvious to newcomers that this is required for running the test suite.
I'm suggesting we make this dependency more explicit in the test suite documentation to help first-time contributors better understand the requirements, not because people are skipping sections. This would help prevent confusion when they follow the test suite instructions step by step.
I'd like to hear from other contributors who are new to Django before making a final decision. Perhaps we could ask in the Django Discord channel to get feedback from newcomers about their experience with the documentation? Their perspective would be valuable in making the documentation more accessible. What do you think?

@timgraham
Copy link
Member

Why would the step be in the tutorial if it's not necessary? We could move that step into this section (would that solve your concern?), however, these steps require cloning Django... which is in the previous section. So I don't understand why the install step is "special" in this respect.

There is also: https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/unit-tests/#quickstart

@nessita
Copy link
Contributor
nessita commented Jan 21, 2025

I'm afraid I still don't understand how a reader ends up at "Running Django's test suite for the first time" while skipping part of the previous section. Isn't the purpose of the proposed clarification aimed at someone who did?

I understand your perspective, Tim, but I also see where the author is coming from. As an experienced contributor, it may seem like the clarification is unnecessary. However, for someone new to Django, it's not immediately intuitive that Django needs to be installed with -e in order to run the tests. In my experience, common Python/Django projects don't require this step: the typical process would involve cloning the repo, setting up the environment with dependencies (without the use of -e), and then running tests with the project's test runner (a tweak of PYTHONPATH may be needed sometimes).

The mental split here is between "using" Django in an env (which requires -e and makes sense) vs "running tests for the project" (where the -e flag is not obvious). In fact, the first time I encountered the -e flag and learned its significance was when I began my work as a Django Fellow! 🤯

In summary, I think we can add the briefest clarification similar to what's already present a few lines above with some tweaks:

Given the previously created virtual environment which includes :ref:`the
local copy of Django in editable mode <intro-contributing-install-local-copy>`, ...

@baidoosik
Copy link
Contributor Author

@timgraham
As you mentioned, it’s natural to follow the steps outlined earlier. However, in my case, I didn’t run $ python -m pip install -e /path/to/your/local/clone/django/ at that point. Instead, I proceeded with the "Creating projects with a local copy of Django" section and applied that command to the Django app I was testing. Maybe My understanding is not enough at that time.

@timgraham
Copy link
Member

I see that dee687e may have added a distracting aside that broke up the flow of the tutorial. (Interesting in light of recent efforts to remove extra details from the other tutorials.) I have nothing more to say so feel free to proceed however you like.

@nessita
Copy link
Contributor
nessita commented Jan 22, 2025

Thank you Tim for your valuable view and sharing your strong experience with fine-tuning docs. I agree that tutorials should not include distracting information but, to me, the reiteration of installing Django locally in editable mode is a reinforcement of a basic concept, not a distracting note.

To explain myself, consider this analogy: when teaching someone how to ride a bike, you’d repeat key points like "keep your eyes focused on the road to help with balance" (mostly to avoid a fall, which is quite desirable in this scenario). This is useful, but if one would also explain how balance works, this definitely is distracting to the learner (and unnecessary at this stage). Similarly, in this tutorial, I agree that reiterating the need to install Django locally with -e is an important reminder for clarity and to avoid "a fall" (an blocking error). The distracting bits would be, for example, going into the mechanics of how -e operates, that indeed would be distracting and overcomplicate things without a clear gai 8000 n.

@baidoosik Could you please consider a broader review of the two sections and ensure consistency, specifically when including "reminders" for the -e need, we do so in a concise and to the point manner? To avoid distracting details but always ensuring proper guardrails in the learner's path. And one more request, while clarity is highly rated, we also value minimizing changes to avoid invalidating existing translations of these pages.

…up in docs.

Added missing 'python -m pip install -e ..' command to test suite setup in docs.
@timgraham
Copy link
Member

@nessita I referred to dee687e as a potentially distracting aside that may not be consistent with the Diátaxis guidelines for tutorials. My thought was that without that aside, the step by step nature of the tutorial may be more obvious. In fact, if I understand correctly, Jordan said in (#19068 (comment)) that he was confused by the aside ("Instead, I proceeded with the "Creating projects with a local copy of Django" section...") and that's why he skipped the required install command.

@baidoosik
Copy link
Contributor Author
baidoosik commented Jan 22, 2025

I partially agree with @timgraham opinion. While that section is helpful, I feel it slightly confuses the purpose of installing Local Dev Django. It’s as if the flow of thought is being disrupted, so I’ve been considering whether it might be better to separate that section(style), as shown in the image below.

image

@baidoosik
Copy link
Contributor Author

@nessita @timgraham I updated code. If you have any feedback, feel free to tell me. I want to make the document as accurate and easy to understand as possible.

@baidoosik baidoosik requested a review from nessita January 23, 2025 00:19
@baidoosik
Copy link
Contributor Author

@nessita gentle reminder 🙏

@nessita
Copy link
Contributor
nessita commented Feb 3, 2025

@nessita gentle reminder 🙏

This is in my review queue but there are other Fellow tasks that have more priority, sorry. I will get to this as soon as possible (but it could take weeks).

@baidoosik
Copy link
Contributor Author

@nessita Are you still busy?

@sarahboyce
Copy link
Contributor

If I was to make changes I would:

--- a/docs/intro/contributing.txt
+++ b/docs/intro/contributing.txt
@@ -177,7 +177,7 @@ Go ahead and install the previously cloned copy of Django:
 
 The installed version of Django is now pointing at your local copy by installing
 in editable mode. You will immediately see any changes you make to it, which is
-of great help when writing your first contribution.
+of great help when testing your first contribution.

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

Successfully merging this pull request may close these issues.

5 participants
0