Skip to content
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

Split dependencies based on required use #30

Merged
merged 10 commits into from
May 9, 2024
Merged

Conversation

meshy
Copy link
Contributor

@meshy meshy commented May 9, 2024

We previously had one set of dependencies which we used for inside tox and outside of it.

This caused a bit of a muddle. Inside tox we couldn't have Django or Psycopg installed (to avoid clashes with the build matrix) and outside of it we needed them so that the tests and linters actually worked.

This splits up the dependencies based on their use.

In the process, it gets Mypy running again in CI, which it seems was broken for some time. I've updated the type ratchet with the now-discovered errors, and will address them separately. This is a good example of the type ratchet in action!

CI caching is fixed too, which speeds up tox quite a bit.

meshy added 3 commits May 9, 2024 11:29
Packaging is done in CI, and doesn't require the makefile.
Before this change, these workflows didn't track all of the important
files which contain python requirements.
@meshy meshy force-pushed the makefile-and-requirements branch from 4e70dee to 6e00c65 Compare May 9, 2024 11:32
@meshy meshy marked this pull request as ready for review May 9, 2024 11:35
@meshy meshy mentioned this pull request May 9, 2024
@meshy meshy requested a review from a team May 9, 2024 11:36
makefile Outdated Show resolved Hide resolved
meshy added 6 commits May 9, 2024 12:40
Before this change, we were using the same requirements file for local
development as we were for running pytest in tox.

This has been causing issues because of the different needs of the
different environments.

This change separates the requirements.
Before this change, we installed a lot of requirements that we didn't
need when we did a release.

This change creates a new more limited requirements file, and uses it
only for releasing.
Before this change, we installed tox directly in CI, but didn't specify
a version, or pin transitive dependencies.
We didn't install these before because they clashed with the
requirements of running Pytest in Tox. This caused issues where
developers and scripts expected these to be installed.

Now that we have isolated the dependencies of pytest-in-tox, we can
install these for local development, and in CI when running linters.

This causes the Mypy linter to begin failing. It reveals a
previously-hidden bug that I will address separately.
These were getting hard to read and differentiate.
Before now, these instructions were unclear and faulty.

This change fixes the pytest instructions, and removes the instructions
for running tox in isolation.
@meshy meshy force-pushed the makefile-and-requirements branch from 6e00c65 to c81e240 Compare May 9, 2024 11:42
Copy link
Contributor Author

@meshy meshy May 9, 2024

Choose a reason for hiding this comment

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

I'll address these issues in another PR. It's great that we're seeing the ratchet in action now!

Before this change, we were seeing a warning when these lines were run:

    WARNING: --strip-extras is becoming the default in version 8.0.0. To
    silence this warning, either use --strip-extras to opt into the new
    default or use --no-strip-extras to retain the existing behavior.
@meshy meshy force-pushed the makefile-and-requirements branch from c81e240 to 46f3a1e Compare May 9, 2024 11:59
@meshy meshy merged commit ebb1bc1 into main May 9, 2024
9 checks passed
@meshy meshy deleted the makefile-and-requirements branch May 9, 2024 12:57
@meshy meshy mentioned this pull request May 9, 2024
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.

2 participants