mkleehammer / pyodbc

Python ODBC bridge
https://github.com/mkleehammer/pyodbc/wiki
MIT No Attribution
2.92k stars 561 forks source link

Add type hints to parameters that are pytest fixtures #1261

Closed keitherskine closed 1 year ago

keitherskine commented 1 year ago

@mkleehammer , this PR isn't crucial, but I think it's a good habit to get into.

"Fixtures" can be included in pytest test functions simply by including the fixture by name as a parameter. Unfortunately, this makes it impossible for linters to deduce the type of the fixture parameter. Hence, it's good practice to provide the type of the fixture within the test function signature. For example, the fixture called "cursor" has a type of pyodbc.Cursor so include that as a type hint.

Also (minor), both @pytest.fixture and @pytest.fixture() can be used in the fixture decorator, but I believe @pytest.fixture() is the preferred use because fixture decorators can take parameters. No biggie though.

Lastly, there were a few print() statements in the unit tests, which I have deleted. I'm pretty sure they shouldn't be there, but happy to be corrected on that.

mkleehammer commented 1 year ago

Thanks Keith. I didn't realize that would fix it - that's good to know. In the past I've usually added a comment to the top, but it is different for each linter. I've added config files for flake8 and pylint, but I take it you are using ruff. (I tried ruff when it was early and it is certainly fast but didn't catch nearly as many issues as pylint. Is it working well for you?)

I'm not sure these fixtures even bring value over calling a simple function. The close calls happen when the test function exits anyway due to reference counting.

keitherskine commented 1 year ago

Type hints are certainly handy. I've become quite the convert over the past year. It was adding the type hints to the unit test functions that allowed me to see that Connection.closed was missing from pyodbc.pyi.

FYI, here's some type hints I use for common pytest fixtures like mocker, caplog, etc.:

import pathlib

import pytest
from pytest_mock import MockerFixture

def test_xxxxxx(capsys: pytest.CaptureFixture,
                capsysbinary: pytest.CaptureFixture,
                caplog: pytest.LogCaptureFixture,
                monkeypatch: pytest.MonkeyPatch,
                tmp_path: pathlib.Path,
                mocker: MockerFixture):

As for ruff. Yes, it's kinda the new kid on the block but seems to do a very good job. For reference, I've attached the ruff.toml config file I use. There are a LOT of checks in ruff, and some of them are a bit pedantic/nonsensical so I suppress them. Personally, I use linters as suggestions rather than hard rules of coding, but it's a personal preference.

my ruff.toml config file