Repository navigation
Move eval files into a separate directory - #102
Conversation
a5594bb to
7c00e15
Compare
jakkdl
left a comment
jakkdl
left a comment
There was a problem hiding this comment.
This is overall a pretty simple PR, with only ~30 lines changed and a bunch of files moved without any edits to them. I don't see much risk of unexpected consequences, since that should be caught by tests not running properly (which happened plenty while writing this 😁), and there should be zero functionality change or anything noticeable for the end user of the plugin.
| hooks: | ||
| - id: flake8 | ||
| args: ["--exclude", ".*,tests/trio*.py"] | ||
| args: ["--exclude", ".*,tests/eval_files/*"] |
There was a problem hiding this comment.
Changing the linter exclude since new location
| hooks: | ||
| - id: flake8 | ||
| args: ["--exclude", ".*,tests/trio*.py", "--select=E800"] | ||
| args: ["--exclude", ".*,tests/eval_files/*", "--select=E800"] |
There was a problem hiding this comment.
Unfortunately still duplicated hook so need to change here as well
| To check that all codes are tested and documented there's a test that error codes mentioned in `README.md`, `CHANGELOG.md` (matching `TRIO\d\d\d`), the keys in `flake8_trio.Error_codes` and codes parsed from filenames matching `tests/trio*.py`, are all equal. | ||
| To check that all codes are tested and documented there's a test that error codes mentioned in `README.md`, `CHANGELOG.md` (matching `TRIO\d\d\d`), the keys in `flake8_trio.Error_codes` and codes parsed from filenames and files in `tests/eval_files/`, are all equal. | ||
|
|
||
| ## Test generator | ||
| Tests are automatically generated for files named `trio*.py` in the `tests/` directory, with the code that it's testing interpreted from the file name. The file extension is split off, if there's a match for for `_py\d*` it strips that off and uses it to determine if there's a minimum python version for which the test should only run. | ||
| Tests are automatically generated for files in the `tests/eval_files/` directory, with the code that it's testing interpreted from the file name. The file extension is split off, if there's a match for for `_py\d*` it strips that off and uses it to determine if there's a minimum python version for which the test should only run. |
| [tool.pyright] | ||
| strict = ["*.py", "tests/test_flake8_trio.py", "tests/conftest.py"] | ||
| exclude = ["tests/trio*.py", "**/node_modules", "**/__pycache__", "**/.*"] | ||
| exclude = ["tests/eval_files/*", "**/node_modules", "**/__pycache__", "**/.*"] |
There was a problem hiding this comment.
Need to change exclude rules here as well for pyright
| def from_filename(cls, filename: str) -> Plugin: | ||
| def from_filename(cls, filename: str | Path) -> Plugin: |
There was a problem hiding this comment.
I changed test_flake8_trio.py to use Path, but since you can just pass that on to tokenize.open it needed no code change. Nothing actually passes a str parameter anymore, but I see no reason not to continue accepting that
There was a problem hiding this comment.
I think to be excruciatingly correct we could specify os.PathLike here, but I think the union is fine 😃
|
|
||
| # TODO: Move test_eval files into a separate directory | ||
| trio_test_files_regex = re.compile(r"trio\d.*.py") | ||
|
|
||
| test_files: list[tuple[str, str]] = sorted( | ||
| (os.path.splitext(f)[0].upper(), f) | ||
| for f in os.listdir("tests") | ||
| if re.match(trio_test_files_regex, f) | ||
| test_files: list[tuple[str, Path]] = sorted( | ||
| (f.stem.upper(), f) for f in (Path(__file__).parent / "eval_files").iterdir() | ||
| ) |
There was a problem hiding this comment.
No need for a regex, and using Path functions to do os.path.splitext (.stem) and os.listdir (.iterdir()).
It now also runs regardless of your current working directory, while previously it required being in the project root.
| def read_file(test_file: str): | ||
| filename = Path(__file__).absolute().parent / test_file | ||
| return Plugin.from_filename(str(filename)) | ||
|
|
||
|
|
There was a problem hiding this comment.
With moving everything to using Path this function can now be completely removed 🎉
| with open(os.path.join("tests", path), encoding="utf-8") as file: | ||
| with open(path, encoding="utf-8") as file: |
There was a problem hiding this comment.
with path being a Path this can be simplified
| plugin = read_file(path) | ||
| plugin = Plugin.from_filename(path) |
There was a problem hiding this comment.
With Plugin.from_filename taking a Path, and path containing the full path, we don't need the helper function at all here.
| def test_noerror_on_sync_code(test: str, path: str): | ||
| def test_noerror_on_sync_code(test: str, path: Path): | ||
| if any(e in test for e in error_codes_ignored_when_checking_transformed_sync_code): | ||
| return | ||
| with tokenize.open(f"tests/{path}") as f: | ||
| with tokenize.open(path) as f: |
There was a problem hiding this comment.
More simplifying thanks to Path.
| def from_filename(cls, filename: str) -> Plugin: | ||
| def from_filename(cls, filename: str | Path) -> Plugin: |
There was a problem hiding this comment.
I think to be excruciatingly correct we could specify os.PathLike here, but I think the union is fine 😃
|
TIL about |
Taking the opportunity while there are no other open pull requests to move all the test_eval files to a separate directory. I've now also seen the light, and converted all paths from strings to Path. 🧘