pyproject: standardise project layout by shtrom · Pull Request #2 · mozilla-conduit/reviewer-selector

shtrom · 2026-03-13T06:19:35Z

pyproject: standardise project layout
deps: add pip-tools to generate requirements.txt
deps: generate requirements[-dev].txt
lint: replace pytest-ruff with test_formatting.py using both black and ruff
pyproject: add setuptools-scm build-system
test: e2e test with exported script name
deps: update requirements
docker: install app using pip
workflows: run tests in container
docker: add REPO_URL and ORG_NAME support

cgsheeh · 2026-03-13T14:23:43Z

pyproject.toml

+dev = [
+  "pip-tools",
+  "pytest",
+  "pytest-ruff",


pytest-ruff looks like a small, mostly unmaintained project (last activity 9 months ago, adding Python 3.13 support). We should roll our own formatting tests as we do in other projects.

Maybe it's just feature complete? 😆

Lando uses both Black and Ruff. Why do we use both and not just one?

cgsheeh · 2026-03-13T14:25:45Z

pyproject.toml

+  "rs_parsepatch",
+]
+description =  "Select reviewers based on a diff and a set of rules"
+dynamic = ["version"]


I think this field only needs to be set when using setuptools-scm. Perhaps we could add that now.

I thought it would, and was surprised that it... just worked? (TBF, I haven't checked the version anywhere, but it just didn't choke on it).

I'm not sure we need setuptools at all considering we don't build sdist-type files.

OTOH, I have just tried adding a tag now, and the version is still 0.0.0 with or without setuptools-scm.

cgsheeh · 2026-03-13T14:29:02Z

requirements.txt

As this package is intended to be a library/client-side CLI tool (as opposed to server-side software) I don't think we need a requirements.txt. Specifying the requirements in pyproject.toml will allow the build to find packages that work for the client given other venv conflicts, as long as we ensure we specify any incompatible versions.

requirements-dev.txt is still valid for running local tests and builds against a known-good version.

I think it's gonna be server-side: we build a docker container that will be used in response to hooks. If anything, as we refine the reviewer selection, this will be used as the shell to potentially bring in other libraries, but not the other way round.

It is, however, designed as a CLI for ease of testing in isolation, but I think having the requirements.txt here helps too, to recreate a known-good environment for the CLI.

cgsheeh · 2026-03-13T14:33:32Z

tests/test_reviewer_selector.py

    resolve_reviewers,
 )

+MAIN_SCRIPT = "src/reviewer_selector.py"


If you want to test the script end-to-end, we should simply execute the name as defined in pyproject.toml:project.scripts (ie subprocess.run(["reviewer-selector"... in this case). Alternatively we could run the main() method directly. Executing the script in this way is brittle as changing the project structure will cause unrelated test failures. It also doesn't reflect any changes to the environment that may happen as part of the build/install.

I'll go with the installed script name, rather than the main, to be sure to test the whole flow (including correct installation of the script).

cgsheeh · 2026-03-13T14:35:16Z

docker/Dockerfile

If this Docker container is used for running tests, perhaps we should use it in the GHA workflow instead of installing requirements-dev.txt and running pytest directly.

This container is the main artifact that gets run by the TC hook. It's very very lean at the moment, but yeah... I'll see if I can keep the test dependencies out of the container.

…d ruff

addressed

* REPO_URL allows to fetch rules from the target repo * ORG_NAME allows to prefix review groups correctly

cgsheeh

LGTM!

cgsheeh · 2026-03-20T00:34:54Z

docker/Dockerfile

+COPY . /app
+RUN pip install -r requirements.txt \
+  && pip install /app


Nit: it would be better to COPY the requirements file in and build the venv first, then copy the app code. That way we only re-build the container when the requirements.txt file changes, instead of each time the app code changes.

This is a nit as it's not super important for an MVP project. :)

Ah, I didn't think of that. In a previous installment I had a RUN --mount, but I removed it when I ended up COPYing everything. I'll restore that!

cgsheeh · 2026-03-20T00:36:32Z

docker/entrypoint.sh

@@ -0,0 +1,73 @@
+#!/bin/sh -eu


Small non-blocking request: could I trouble you to convert this into an entrypoint.py instead? As our team's stack is Python, it seems it would be easier to maintain. I can generally follow what's happening in this script, but if there was a subtle shell-script bug here I would have a hard time spotting it. :)

Ah, yeah. That shell script kinda grew a bit in size. I think the rewrite will happen naturally in the next few PR as I integrate TC and GitHub, and Python code ultimately becomes a shell around mots.

cgsheeh · 2026-03-20T00:38:51Z

README.md

-to set the reviewers on the target PR.
+* If `DIFF_URL` is given, it will be fetched and passed into the selector's
+stdin.
+* If `GITHUB_TOKEN` and `PR_URL` are provided, the container will attempt


Suggested change

* If `GITHUB_TOKEN` and `PR_URL` are provided, the container will attempt

* If `GITHUB_TOKEN` and `PR_URL` are provided, the container will attempt to set the reviewers on the target PR.

Ah! This one travelled all the way to the bottom of the file!

cgsheeh · 2026-03-20T00:40:16Z

pyproject.toml

+  "black",
+  "pip-tools",
+  "pytest",
+  "ruff",


Nit: it might be worth pinning these to specific versions.

What would be the rationale here? We are hard-pinned by the requirements.txt, so my instinct would be to have nothing more than a loose pinning here, likely if we encounter incompatibility with newer versions that we can't fix immediately.

shtrom force-pushed the pyproject-setup branch 4 times, most recently from 50f6d81 to 3d3132e Compare March 13, 2026 06:39

shtrom requested a review from a team as a code owner March 13, 2026 06:39

shtrom force-pushed the pyproject-setup branch 3 times, most recently from 1b86870 to 6b5fb45 Compare March 13, 2026 06:49

shtrom added 3 commits March 13, 2026 17:53

pyproject: standardise project layout

d2d3a4d

deps: add pip-tools to generate requirements.txt

6c359a0

deps: generate requirements[-dev].txt

e17b19e

shtrom force-pushed the pyproject-setup branch from fe53d55 to e17b19e Compare March 13, 2026 06:53

cgsheeh previously requested changes Mar 13, 2026

View reviewed changes

shtrom added 6 commits March 16, 2026 16:27

lint: replace pytest-ruff with test_formatting.py using both black an…

4b27a91

…d ruff

pyproject: add setuptools-scm build-system

33271e5

test: e2e test with exported script name

1a81f99

deps: update requirements

f6e6589

docker: install app using pip

8067586

workflows: run tests in container

958eebb

shtrom force-pushed the pyproject-setup branch from f5ed57d to 958eebb Compare March 16, 2026 05:52

Merge remote-tracking branch 'origin/main' into pyproject-setup

9a571c9

shtrom requested a review from cgsheeh March 18, 2026 05:24

docker: add REPO_URL and ORG_NAME support

8c87df5

* REPO_URL allows to fetch rules from the target repo * ORG_NAME allows to prefix review groups correctly

shtrom force-pushed the pyproject-setup branch from 352f6f1 to 8c87df5 Compare March 18, 2026 08:06

cgsheeh approved these changes Mar 20, 2026

View reviewed changes

shtrom added 3 commits March 20, 2026 15:25

fixup! docker: install app using pip

8511c40

fixup! docker: add REPO_URL and ORG_NAME support

cc9693c

fixup! docker: add REPO_URL and ORG_NAME support

99bd08d

	* If `GITHUB_TOKEN` and `PR_URL` are provided, the container will attempt
	* If `GITHUB_TOKEN` and `PR_URL` are provided, the container will attempt to set the reviewers on the target PR.

Conversation

shtrom commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shtrom Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shtrom Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgsheeh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

shtrom commented Mar 13, 2026 •

edited

Loading

shtrom Mar 16, 2026 •

edited

Loading

shtrom Mar 16, 2026 •

edited

Loading