maint: add pre-commit, mypy, testing docker#473
Conversation
There was a problem hiding this comment.
Thanks for the work, I'm curious to see what you changed :)
This PR looks fine, and I like that formatting is done in a separate PR for easier reviewing. Some notes:
I think we should exclude e2e_projects and tests/data/test_generation folders, the changes there caused the tests to fail. Some of the code there has "perfect imperfections" (as noted in the comments, we want to test that mutmut can handle code stretching over multiple lines, and stuff like Union). I don't really mind if the code quality there is bad.
The pre-commit version is pretty outdated (~3 years), is there a particular reason for this? If using an up-to-date version would cause merge-conflicts, we can postpone upgrading until you submitted the other PRs.
And small documentation nitpick: I like that formatting is not validated as part of the CI, so we keep it simple for new contributors. However, I think we could add pre-commit to the dev-dependencies (uv add --dev pre-commit ), and mention pre-commit install in the CONTRIBUTING page for motivated users
e2e_projects/mutate_only_covered_lines/src/mutate_only_covered_lines/__init__.py
Show resolved
Hide resolved
|
@Otto-AA thanks for the review, I have the complete public set of changes here https://github.com/lyft/mutmut/tree/nicklafleur/oss-complete that you can check out. It's quite substantial (+15k, -3k) but everything is very malleable in my mind and open to debate. We are by no means blocked internally so happy to evolve things in a measured way here. Working on your suggestions now :) |
nicklafleur
force-pushed
the
nicklafleur/pre-commit
branch
3 times, most recently
from
90a1798 to
6bb6a62
Compare
|
I believe I've addressed all the changes. All the snapshots being changed to segfault codes is due to setproctitle which I have a fix coming for in a future PR. Happy to just reset it to the remote version if you would prefer. |
nicklafleur
force-pushed
the
nicklafleur/pre-commit
branch
from
6bb6a62 to
0cefa52
Compare
nicklafleur
force-pushed
the
nicklafleur/pre-commit
branch
from
0cefa52 to
804fef9
Compare
|
Thanks, I'll try to take a look tomorrow.
Yes, I'd prefer if the CI continues to pass (which runs on Linux). If it's easier for you, you can also pytest.mark.skipif the test on MacOS and also could add a temporary copy of the test that only runs on MacOS with the segfault errors. |
nicklafleur
force-pushed
the
nicklafleur/pre-commit
branch
from
804fef9 to
b9c454c
Compare
No problem, should have thought to spin up a container for this anyways, let me just do that. |
nicklafleur
force-pushed
the
nicklafleur/pre-commit
branch
from
5817014 to
e4683da
Compare
nicklafleur
changed the title
Additions: - /docker/Dockerfile.test which runs the tests inside a linux container with python 3.10 - /scripts/run_tests.sh runs the above Documentation: - How to use the above
Adds all type annotations to satisfy mypy. There should be 0 functional changes.
nicklafleur
force-pushed
the
nicklafleur/pre-commit
branch
from
e4683da to
92e54ad
Compare
|
|
||
| ENV UV_PROJECT_ENVIRONMENT=/opt/venv | ||
|
|
||
| COPY . . |
There was a problem hiding this comment.
Just FYI, this will cause any change to the source code to re-execute the RUN uv sync --group dev command and download the packages. There are patterns to make this more efficient (iirc only copy pyproject.toml + uv.lock first, then sync, then copy the rest / use a volume mount).
So if you want better performance, you could change that. I don't mind it being like this though
There was a problem hiding this comment.
good call, I'll add this to the next pr
|
Thanks for the update, looks good and even with type checking ❤️ |
|
No problem! mypy was actually part of a future commit but figured it would be better to just have all linting in the same PR. |
2 participants
👋 Hi there mutmut maintainers.
We at Lyft have been tinkering internally with mutmut as a means to integrate some new approaches to testing and test quality evaluation. We've made some pretty considerable refactors to the framework that we would like to start merging to the upstream, but wanted to start with some housekeeping items as a show of good faith and thanks for all your work so far.
A sneak peak of some of the changes we have made so far are (all broken down into manageable commits and PRs of course):
Enums,@classmethods, and@staticmethodsPlease feel free to reach out to me directly at nicholaslafleur@lyft.com if you would like to discuss.