Repository: AustinScola/pytest-motor (language: python)

Issue:
Workflow upgrade
Current workflow is pretty good, but also a mess. Let me explain:

## Problems

1. There is no centralized source of truth for dependencies. I'd prefer one `requirements.txt` file, `pip install -r requirements.txt` and I'm ready for development. I also don't see any pros for creating separated venvs, as it's only taking more disk space and makes you switch between them regularly.
2. Scripts! As for now, project on 1/3 consists of scripts. Scripts are hard to maintain, they are platform-dependent, and you have to run them manually. IMHO, they MUST be replaced.

## Suggestions

We can:

- [ ]  Use [`poetry`](https://python-poetry.org/) to manage dependencies & venvs (it seems to support building and publishing as well).
- [ ]  Use [`tox`](https://tox.readthedocs.io/en/latest/) to standardize testing. For example, see [`pytest-asyncio`](https://github.com/pytest-dev/pytest-asyncio).
- [x]  Place most of the configs in `pyproject.toml`.
- [ ]  Setup multiplatform development workspace with [`devcontainer.json`](https://docs.github.com/en/codespaces/getting-started-with-codespaces/getting-started-with-your-python-project-in-codespaces). Also, `vs-code` Python extension can take care of linting and formatting.

## Afterword

I know it isn't so simple as "do that and everyone will be happy" so leave your thoughts in the comments.


Discussion / hints:
Hey Alex, thanks for the feedback. I'm very aware that bash scripts are not a good long term solution. It was simply copy/pasted from some of my other projects where it is actually a pretty good solution for getting up and running where it is just me working on them.

Let me first explain a bit about the current design. The reason for having multiple requirements files is that it enables only installing the requirements that are actually needed for any particular task. This is especially helpful in the GItHub Action workflows. Each job can just install the needed requirements. I would push back on there not being any single source of truth. The single source of truth for any script is the generated frozen requirements file. You are correct that it takes more disk space though.

In terms of setting up venvs or switching between them, I don't think that is something users should _ever_  do. So that is why the scripts bootstrap the venvs for you. All a dev has to do is run `./scripts/test.sh` for example and a venv with all the needed requirements will be set up, then pytest will be run in that venv.

One downside as you mention is maintainability. Another that I think is important is that the current set up doesn't allow for testing against multiple versions of dependencies (or Python versions). This script set up was copied from other projects of mine where this isn't a problem because there are no downstream users downloading or depending on the project (the code just runs on servers).

All that said, I agree that the scripts should be replaced. I have only limited knowledge of poetry and tox but I have been considering them already. (I also am interested in writing my own bootstrapper framework...). Have you used either of them before? Do you have any preferences?

Centralizing the configs is also something I have considered. Though I was under the impression that `pyproject.toml` is becoming a somewhat standard?

I will have to read up on `devcontainer.json`. Looks like it is for GitHub Codespaces which is still in beta?

Is this overhaul something that you would be interested in working on?
> Each job can just install the needed requirements.

I haven't had a lot of experience with GitHub Actions (I had a **lot** with GitLab CI/CD, though), but I think those special dependencies can be configured within the job config file.

> ...current set up doesn't allow for testing against multiple versions of dependencies (or Python versions).

`Tox` seems to solve that exact problem.

> Have you used either of them before? Do you have any preferences?

I'm only started studying both of them and already fell in love😄.

> Though I was under the impression that `pyproject.toml` is becoming a somewhat standard?

`pyproject.toml` is a really cool solution too! I think the reason for it becoming a standart is [PEP 518](https://www.python.org/dev/peps/pep-0518/).

> I will have to read up on `devcontainer.json`. Looks like it is for GitHub Codespaces which is still in beta?

It is GitHub Codespaces / VS Code thing. It lets you run the project on the web or using VS Code, with all needed configuration (for example: installed formatters and linters) already been applied to a Docker container. I think it's really cool, because all of us will develop in the exact same environment.

> Is this overhaul something that you would be interested in working on?

Surely yes! I can create a PR for that, but I think it's better to release a new version first and maybe put something like "under refactoring" label in ReadMe, so other contributors will know that their PRs will probably need merge-conflict resolving. Btw, how about adding badges to ReadMe?

> I think those special dependencies can be configured within the job config file.

Not sure what you mean by this?

> I'm only started studying both of them and already fell in love.

Would you like to make the decision about which tool to use?

> It is GitHub Codespaces / VS Code thing.

Ahh cool. I'm excited to try codespaces. I don't use VS Code but if it helps use it then we can add it to the repo.

> maybe put something like "under refactoring" label in ReadMe

I don't think that the infra changes would impact other development of the code?

+1 to README badges
> > I think those special dependencies can be configured within the job config file.
> 
> Not sure what you mean by this?

For example, we now store `wheel` in `basic_requirements.txt`. However, we can move it directly to `.github/workflows/something.yaml`. Here is an example from `pytest-asyncio` GitHub Actions config.

```main.yaml
steps:
      - uses: "actions/checkout@v2"
      - uses: "actions/setup-python@v2"
        with:
          python-version: "${{ matrix.python-version }}"
      - name: "Install dependencies"
        run: |
          set -xe
          python -VV
          python -m site
          python -m pip install --upgrade pip wheel
          python -m pip install --upgrade coverage[toml] virtualenv tox tox-gh-actions
      - name: "Run tox targets for ${{ matrix.python-version }}"
        run: "python -m tox"
```

> > I'm only started studying both of them and already fell in love.
> 
> Would you like to make the decision about which tool to use?

They are solving different problems, aren't they? We can use `tox` for test automation and `poetry` to manage dependencies and build the package. Here is an [article](https://levelup.gitconnected.com/python-a-toxic-work-environment-tox-poetry-b8200563c474) that I've found, that describes the process of setting up both of them.

> > maybe put something like "under refactoring" label in ReadMe
> 
> I don't think that the infra changes would impact other development of the code?

Until they add new dependencies... Yeah, adding this label seems to be an overkill 🤔 


If you had something like this in a workflow

> ```yaml
>       - name: "Install dependencies"
>         run: |
>           set -xe
>           python -VV
>           python -m site
>           python -m pip install --upgrade pip wheel
>           python -m pip install --upgrade coverage[toml] virtualenv tox tox-gh-actions
>       - name: "Run tox targets for ${{ matrix.python-version }}"
>         run: "python -m tox"
> ```

then what would you tell users to run locally? All those same steps? Wouldn't it be best to wrap that in a script in which case we are back where we started?

> They are solving different problems, aren't they? We can use `tox` for test automation and `poetry` to manage dependencies and build the package. Here is an [article](https://levelup.gitconnected.com/python-a-toxic-work-environment-tox-poetry-b8200563c474) that I've found, that describes the process of setting up both of them.

Unfortunately that article is paywalled for me?
> then what would you tell users to run locally?

I don't think most of the contributors need to build the package on their machines. However, if they do, this should be quite simple if we use `poetry`. With `poetry` GitHub Actions and `requirements` files will look differently, so this discussion is a bit pointless. Probably it's just better to create a PR and see if it works.

> Unfortunately that article is paywalled for me?

Oops... I can quickly retell it for you :)

The main statement of the article is that while `poetry` helps you with installing dependencies for the development, `tox` takes care of the dependencies while testing + adds ability to run tests against multiple python versions. They complement each other really well. And here is an [example](https://github.com/dbudwin/MediumBlogExamples/tree/master/ToxicWorkEnvironment) from the article.






Fix patch:
diff --git a/.coveragerc b/.coveragerc
deleted file mode 100644
index 6f4898d..0000000
--- a/.coveragerc
+++ /dev/null
@@ -1,2 +0,0 @@
-[report]
-fail_under=78
diff --git a/.isort.cfg b/.isort.cfg
deleted file mode 100644
index 0d6a8e7..0000000
--- a/.isort.cfg
+++ /dev/null
@@ -1,4 +0,0 @@
-[settings]
-quiet=True
-line_length=100
-skip_glob=venvs,build
diff --git a/.pylintrc b/.pylintrc
deleted file mode 100644
index 66e6362..0000000
--- a/.pylintrc
+++ /dev/null
@@ -1,3 +0,0 @@
-[MESSAGE CONTROL]
-enable=useless-suppression
-disable=duplicate-code
diff --git a/.style.yapf b/.style.yapf
deleted file mode 100644
index b050851..0000000
--- a/.style.yapf
+++ /dev/null
@@ -1,2 +0,0 @@
-[style]
-column_limit=100
diff --git a/.yapfignore b/.yapfignore
index 186e040..891d6f7 100644
--- a/.yapfignore
+++ b/.yapfignore
@@ -1,2 +1,2 @@
 venvs
-build
+build
\ No newline at end of file
diff --git a/mypy.ini b/mypy.ini
deleted file mode 100644
index 6210fd9..0000000
--- a/mypy.ini
+++ /dev/null
@@ -1,28 +0,0 @@
-[mypy]
-python_version = 3.6
-warn_return_any = True
-warn_unused_configs = True
-exclude=venvs/
-
-disallow_untyped_defs=True
-
-no_implicit_optional=True
-
-warn_unused_ignores=True
-
-show_error_codes=True
-
-[mypy-setuptools.*]
-ignore_missing_imports = True
-
-[mypy-motor.*]
-ignore_missing_imports = True
-
-[mypy-pymongo.*]
-ignore_missing_imports = True
-
-[mypy-distro.*]
-ignore_missing_imports = True
-
-[mypy-pytest_lazyfixture.*]
-ignore_missing_imports = True
diff --git a/pyproject.toml b/pyproject.toml
new file mode 100644
index 0000000..de39664
--- /dev/null
+++ b/pyproject.toml
@@ -0,0 +1,45 @@
+[tool.coverage]
+fail_under = 78
+
+[tool.yapf]
+column_limit = 100
+
+[tool.pytest.ini_options]
+addopts = "--strict-markers"
+testpaths = ["tests"]
+markers = [
+    "unit: marks tests as a unit test",
+    "integration: marks tests as an integration test",
+]
+
+[tool.pylint.format]
+max-line-length = 100
+
+[tool.pylint.messages_control]
+enable = ["useless-suppression"]
+disable = ["duplicate-code"]
+
+[tool.mypy]
+python_version = "3.6"
+warn_return_any = true
+warn_unused_configs = true
+exclude = "venvs/"
+disallow_untyped_defs = true
+no_implicit_optional = true
+warn_unused_ignores = true
+show_error_codes = true
+
+[[tool.mypy.overrides]]
+module = [
+    "setuptools.*",
+    "motor.*",
+    "pymongo.*",
+    "distro.*",
+    "pytest_lazyfixture.*",
+]
+ignore_missing_imports = true
+
+[tool.isort]
+quiet = true
+line_length = 100
+skip_glob = ["venvs", "build"]
diff --git a/requirements/format_requirements.txt b/requirements/format_requirements.txt
index 916931b..ef058a5 100644
--- a/requirements/format_requirements.txt
+++ b/requirements/format_requirements.txt
@@ -1,2 +1,3 @@
 isort==5.8.0
 yapf==0.31.0
+toml==0.10.2
\ No newline at end of file
diff --git a/requirements/frozen/frozen_format_requirements.txt b/requirements/frozen/frozen_format_requirements.txt
index 916931b..2e6c738 100644
--- a/requirements/frozen/frozen_format_requirements.txt
+++ b/requirements/frozen/frozen_format_requirements.txt
@@ -1,2 +1,3 @@
 isort==5.8.0
+toml==0.10.2
 yapf==0.31.0


Test patch:
diff --git a/pytest.ini b/pytest.ini
deleted file mode 100644
index 360f861..0000000
--- a/pytest.ini
+++ /dev/null
@@ -1,7 +0,0 @@
-[pytest]
-addopts = --strict-markers
-testpaths =
-    tests
-markers =
-    unit: marks tests as a unit test
-    integration: marks tests as an integration test