docs: Add how-to for Python development (virtualenvs, make test) #392
Conversation
We currently have "Getting Started" instructions in each repo's README, but they're terse and often out of date. By moving them to a central place, we can instead link to those docs and reduce our maintenance burden.
|
|
||
| make test | ||
|
|
||
| Depending on the repository this might run not just unit tests, but also linters and other quality checks. The Makefile may also define a more comprehensive ``test-all`` target. |
There was a problem hiding this comment.
quality is often a separate target and worth mentioning
There was a problem hiding this comment.
There's a huge amount of variation in what the various repos include. Maybe I should just make a list of targets that may or may not be relevant? I think those would be test, test-all, quality, validate, docs, diff_cover, coverage. Some even have targets for isort, pycodestyle, and pylint. I think in at least one, you're just supposed to run tox directly. (In some repos that only runs pytest, and other places it runs all the checks.)
It does seem like make quality is worth mentioning. Maybe nothing beyond that, and let CI checks take care of the rest?
My longer term goal is to homogenize the Makefile targets we use for Python. Writing this how-to really illustrates some of the current problems.
There was a problem hiding this comment.
Yeah I wouldn't make the list, that gets confusing. Make test and make quality work pretty often and running both of those will get you a long way to passing your PR checks.
|
|
||
| #. As you change code and add tests, you can use ``make test`` to check if tests are still passing. | ||
| #. Run ``make test`` one more time and commit your changes with ``git commit``. Follow the `conventional commits`_ documentation. Make sure your commit message is informative and describes why the change is being made. While the first line of the message should be terse, the body of the message has plenty of room for details. | ||
| #. Push your changes to GitHub. If you have write access to the repository, you can run ``git push``. If not, you'll need to fork the repository in GitHub. After forking, you can use ``git remote add fork <fork_url>`` to tell git about the fork, and then ``git push fork`` to push your branch to your fork. |
There was a problem hiding this comment.
If you go through the steps of creating a branch then realize you need a fork, is it annoying at all? Or does it Just Work®?
There was a problem hiding this comment.
It should just work. Unless you check out a remote branch (creating a local one) or use -u/--set-upstream on a local branch, git doesn't have any particular opinion about what remote the branch goes with.
(I usually would use git push -u fork here, so that the branch always pushes to the same remote. Maybe I should include the -u option here?)
There was a problem hiding this comment.
I guess I wonder if it makes sense to instruct people to make a fork first
There was a problem hiding this comment.
I think fork-by-default is a reasonable strategy that aligns with most open source projects on GitHub. I'm an org owner and I use forks for almost everything.
Only maintainers and release managers really need to push new branches upstream, and folks in those roles probably don't need a guide to do that.
There was a problem hiding this comment.
Fair enough, I'll separate it out.
There was a problem hiding this comment.
I haven't seen people using fork as the name of their personal forks. More common is origin for your fork, and upstream for the upstream.
There was a problem hiding this comment.
I've seen both patterns. origin/upstream weirds me out because the upstream is the origin -- they more or less mean the same thing! Frankly, I use both, depending on whether I've initially cloned the authoritative repo or my fork. I'm too lazy to rename the remotes. 🙃
There was a problem hiding this comment.
I've updated the PR to tell people to use a fork. It doesn't tell people how to update their fork, but that might be out of scope.
There was a problem hiding this comment.
I think that is out of scope, but you could consider linking this: https://docs.openedx.org/en/latest/developers/quickstarts/first_openedx_pr.html
There was a problem hiding this comment.
Thanks Tim, looks good.
It doesn't tell people how to update their fork, but that might be out of scope.
I think that's fair. This doc probably isn't the right place for a comprehensive "how to use git with multiple remotes" guide.
|
|
||
| If you create virtualenvs inside repos, you'll need to tell git to ignore them. The easiest way to do this is to create a file called ``.gitignore-global`` in your home directory and add the line ``.venv-3.*/``. Alternatively, you can create the virtualenvs elsewhere in your filesystem. | ||
|
|
||
| Many developers use wrapper scripts (or write their own using shell aliases). One commonly used tool is ``virtualenvwrapper``, which manages the virtualenv directories outside of repositories; this avoids several issues with git and other tools being able to see the virtualenv, but will require explicitly naming each virtualenv. |
There was a problem hiding this comment.
Maybe make this a .. note:: directive and add a link to the virtualenv wrapper docs: https://virtualenvwrapper.readthedocs.io/en/stable/
|
|
||
| #. As you change code and add tests, you can use ``make test`` to check if tests are still passing. | ||
| #. Run ``make test`` one more time and commit your changes with ``git commit``. Follow the `conventional commits`_ documentation. Make sure your commit message is informative and describes why the change is being made. While the first line of the message should be terse, the body of the message has plenty of room for details. | ||
| #. Push your changes to GitHub. If you have write access to the repository, you can run ``git push``. If not, you'll need to fork the repository in GitHub. After forking, you can use ``git remote add fork <fork_url>`` to tell git about the fork, and then ``git push fork`` to push your branch to your fork. |
There was a problem hiding this comment.
+1 to assuming people are working with forks, the plan is to have reduced access for 2U in the future too so having the default docs assume you use forks first will be generally useful.
|
|
||
| #. Make a branch for your changes:: | ||
|
|
||
| git checkout -b <your_github_username>/<short_descriptive_label> |
There was a problem hiding this comment.
Can we recommend git switch -c <github_username>/<short_label>? switch is a newer command that seems like a better user experience.
There was a problem hiding this comment.
FWIW I have been forcing myself to use switch and restore lately, and can confirm that they just make more sense.
There was a problem hiding this comment.
I've never used it, and the man page has a big warning:
THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE.
...but I'm guessing the basic behavior will remain unchanged? It looks like the main benefit is that you don't have to do git stash; git checkout ...; git stash pop when you're on the wrong branch at first.
There was a problem hiding this comment.
I didn't realize there was a stash different with switch. To me, the main advantage is that it's conceptually clearer. I wish they would remove the EXPERIMENTAL warning. There isn't that much behavior to change in the first place.
|
|
||
| The most basic way of creating a virtualenv is to use the builtin ``venv`` module in Python:: | ||
|
|
||
| python3.8 -m venv .venv-3.8 |
There was a problem hiding this comment.
I kind of understand why the -3.8 suffix is there, but these instructions will only involve one venv, so maybe we can keep it simpler by skipping the suffix.
|
|
||
| .. How-tos should have a short introduction sentence that captures the user's goal and introduces the steps. | ||
|
|
||
| This how-to will help prepare your local system to start developing on a Python-based service that is part of the Open edX Platform. |
There was a problem hiding this comment.
| This how-to will help prepare your local system to start developing on a Python-based service that is part of the Open edX Platform. | |
| This how-to will help prepare your local system to start developing on a Python repository that is part of the Open edX Platform. |
This will work on Python libraries too, right?
There was a problem hiding this comment.
Yes, libraries too -- will change.
| .. note:: | ||
|
|
||
| Some repositories will have a variation of these instructions in their README. Often it will be better to follow the instructions here instead, as the README instructions may be outdated, but keep an eye out for repositories that document special workflows. |
There was a problem hiding this comment.
I get what you're saying here, but from the perspective of a new Python dev who has no idea what special workflows are (which IMO is the audience of this doc), this note amounts to saying:
If these instructions conflict with the repository's instructions, then follow these instructions, except in the cases where you shouldn't, and in those cases you should follow repository's instructions.
which I think is more confusing and disheartening than it is helpful 😛
edit: Since I didn't make a clear suggestion--I suggest either taking a firmer stance or nixing the note.
There was a problem hiding this comment.
It's tough to figure out how to say "a lot of those READMEs are wrong but some of them are not" in a useful way that, frankly, isn't too embarrassing. Maybe this:
These instructions will not work in all repositories. If they do not, check the repo's README for a Getting Started section for possible alternative instructions.
There was a problem hiding this comment.
I actually really like this wording!
|
|
||
| make test | ||
|
|
||
| Depending on the repository this might run not just unit tests, but also linters and other quality checks. The Makefile may also define ``make quality`` for code quality checks, or even a comprehensive ``test-all`` target. |
There was a problem hiding this comment.
Gah, I would love to standardize on something. I've become quite partial to tutor's way, which is to namespace all the checks under make test-* and use make test as the omni-check, but I think I'd be happy with any standard at this point.
I know that's outside the scope. I think how you described the current situation here is great.
| git checkout -b <your_github_username>/<short_descriptive_label> | ||
|
|
||
| #. As you change code and add tests, you can use ``make test`` to check if tests are still passing. | ||
| #. Run ``make test`` one more time and commit your changes with ``git commit``. Follow the `conventional commits`_ documentation. Make sure your commit message is informative and describes why the change is being made. While the first line of the message should be terse, the body of the message has plenty of room for details. |
There was a problem hiding this comment.
This skips git add.
You could spell out git add as a step. Alternatively, you could be purposefully vague, and let the developer find a separate resource for learning git basics.
| #. Run ``make test`` one more time and commit your changes with ``git commit``. Follow the `conventional commits`_ documentation. Make sure your commit message is informative and describes why the change is being made. While the first line of the message should be terse, the body of the message has plenty of room for details. | |
| #. Run ``make test`` one more time and then commit your changes. Follow the `conventional commits`_ documentation. Make sure your commit message is informative and describes why the change is being made. While the first line of the message should be terse, the body of the message has plenty of room for details. |
| #. As you change code and add tests, you can use ``make test`` to check if tests are still passing. | ||
| #. Run ``make test`` one more time and commit your changes with ``git commit``. Follow the `conventional commits`_ documentation. Make sure your commit message is informative and describes why the change is being made. While the first line of the message should be terse, the body of the message has plenty of room for details. | ||
| #. Push your changes to GitHub. If you have write access to the repository, you can run ``git push``. If not, you'll need to fork the repository in GitHub. After forking, you can use ``git remote add fork <fork_url>`` to tell git about the fork, and then ``git push fork`` to push your branch to your fork. | ||
| #. In GitHub, open a pull request for your changes and ask for a review. |
There was a problem hiding this comment.
| #. In GitHub, open a pull request for your changes and ask for a review. | |
| #. In GitHub, open a pull request (PR). In the PR description, include anything that could help reviewers understand and test your change. | |
| #. If you have a repository maintainer in mind, you can reach out to them and ask for review. Otherwise, an Open edX project manager will triage your PR and find someone to review it. This process can take some time; we appreciate your patience! |
"Ask for review" only works for folks who know maintainers/CCs. Without getting too deep into "How to make a PR", here are a couple items that could clue them into the next steps.
|
|
||
| .. note:: | ||
|
|
||
| If you create virtualenvs inside repos, you'll need to tell git to ignore them. The easiest way to do this is to create a file called ``.gitignore-global`` in your home directory and add the line ``.venv-3.*/``. Alternatively, you can create the virtualenvs elsewhere in your filesystem. |
There was a problem hiding this comment.
Some (but of course not all) repos already mention venv in their .gitignore files.
There was a problem hiding this comment.
The Python cookiecutter doesn't mention it :(
There was a problem hiding this comment.
I'd proposed that change but closed it since it seemed like I was the only one having that problem: openedx/edx-cookiecutters#79
@nedbat if you want to reintroduce that change again I'll give it a 👍🏻
|
@timmc-edx let me know when you think this is ready to merge and if you'd like me to squash it or retain all the commits. |
|
I think I've addressed all feedback except Kyle's regarding the issue of conflicting instructions. Still pondering what to do there; suggestions very welcome. |
|
OK, I think it's ready to go. |
|
@timmc-edx do you want to squash to something you're happy with or is it cool to just squash all the commits together upon merge? |
|
PR title & desc should be fine for the squashed commit message. |
|
Thanks Tim! 🎉 |
We currently have "Getting Started" instructions in each repo's README, but they're terse and often out of date. By moving them to a central place, we can instead link to those docs and reduce our maintenance burden.
This would also involve updating OEP-55 to specify a link to this doc page rather than suggesting an inlined Getting Started section.