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
| n = 1 \ | ||
| + 2 | ||
| # should mutate keys and values | ||
| d = { |
There was a problem hiding this comment.
I guess these got removed because they are unused? Would be nice to keep them and also keep the original formatting (which tests how mutmut+coverage.py handles weird multiline statements).
In general, I would tend to exclude the e2e_projects from formatting+linting
There was a problem hiding this comment.
good call for sure, will convert back to draft and fix
| def some_func_clone(a, b: str = "111", c: Callable[[str], int] | None = None) -> int | None: pass # pragma: no mutate | ||
|
|
||
| def some_func_clone(a, b: str = "111", c: Callable[[str], int] | None = None) -> int | None: | ||
| pass # pragma: no mutate |
There was a problem hiding this comment.
We also need the # pragma: no mutate on the function definition line (the b: str = "111" gets mutated to b: str = "XX111XX" like this)
| import ctypes | ||
| from collections.abc import Callable | ||
| from functools import cache | ||
| from typing import Union |
There was a problem hiding this comment.
We should keep old constructs like Union in the E2E tests to ensure we still support them
There was a problem hiding this comment.
ahh good call yeah I missed this when reviewing the warnings thanks
pyproject.toml
Outdated
| target-version = "py310" | ||
|
|
||
| [tool.ruff.lint] | ||
| select = ["E", "F", "W", "B", "C4", "UP"] |
There was a problem hiding this comment.
If we add following rules, could we remove the black and autoflake pre-commit hooks?
|
@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
2 times, most recently
from
30b524d to
90a1798
Compare
nicklafleur
force-pushed
the
nicklafleur/pre-commit
branch
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. |
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.