lutris icon indicating copy to clipboard operation
lutris copied to clipboard

Start to use `mypy strict`

Open qarmin opened this issue 1 year ago • 11 comments

By default, mypy only suggest the most basic type hints and problems

mypy --strict

will also report errors like

lutris/gui/application.py:132: error: Call to untyped function "ErrorDialog" in typed context  [no-untyped-call]
lutris/gui/application.py:136: error: Function is missing a return type annotation  [no-untyped-def]
lutris/gui/application.py:136: note: Use "-> None" if function does not return a value
lutris/gui/lutriswindow.py:847: error: "_Child" has no attribute "get_child_by_name"  [attr-defined]
lutris/gui/lutriswindow.py:428: error: Argument 1 to "get_natural_sort_key" has incompatible type "object"; expected "str"  [arg-type]
lutris/gui/dialogs/game_import.py:260: error: Invalid index type "None" for "Dict[str, Dict[str, Collection[str]]]"; expected type "str"  [index]
lutris/startup.py:175: error: Call to untyped function (unknown) in typed context  [no-untyped-call]

qarmin avatar Jan 13 '24 11:01 qarmin

Found 7009 errors in 246 files (checked 278 source files)

We've just introduced type hints in the code base. Maybe it's way too early to enable such options. We're not going to fix 7000 typing errors overnight...

strycore avatar Jan 14 '24 09:01 strycore

Funny, when I add --strict it makes no difference. Is there some trick to this?

danieljohnson2 avatar Jan 14 '24 11:01 danieljohnson2

I can easily reproduce this errors with clean git repo

git clone https://github.com/lutris/lutris.git lt
cd lt
python3 -m venv venv
source venv/bin/activate
pip install mypy
mypy --strict

Of course, adding/fixing such number of types should be split into smaller commits to avoid problems, but also this should be checked in each commit to not reintroduce problems. Except https://github.com/lutris/lutris/blob/master/.travis.yml I could not find other CI, that could test mypy - is there any reason why github CI is not used?

qarmin avatar Jan 14 '24 13:01 qarmin

OK, I fixed it. I had some leftover crap in ~/.local/lib. All gone, now it works.

danieljohnson2 avatar Jan 14 '24 13:01 danieljohnson2

Hmm, now the problem is back. It seems to happen when I get this error:

mypy: can't read file '/home/danj/.local/lib/python3.12/site-packages//google': No such file or directory

That had gone away when I removed the directory, but once I run make sc the error comes back. google-stubs is in there now, but google is not.

I think I am missing something.

danieljohnson2 avatar Jan 14 '24 13:01 danieljohnson2

Hmmmm... I see same problem in - https://github.com/lutris/lutris/pull/5237

 Successfully installed types-PyYAML-6.0.12.12 types-protobuf-4.24.0.20240106 types-requests-2.31.0.20240106 urllib3-2.1.0
mypy: can't read file '/home/runner/.local/lib/python3.10/site-packages//google': No such file or directory
Installing missing stub packages:
/usr/bin/python -m pip install types-PyYAML types-protobuf types-requests

qarmin avatar Jan 15 '24 09:01 qarmin

I may slowly start to add missing types with mypy, where possible(probably the most basic ones), but it would have been a good idea to:

  • merge - https://github.com/lutris/lutris/pull/5237 - this speedups entire code checking step multiple times, also adds CI job to test for regressions
  • run ruff format . on entire codebase to have standardized code formatting

qarmin avatar Jan 15 '24 22:01 qarmin

Have you figured out the problem with the //google directory?

I should warn you before you start adding annotations that Glorious Leader is not fond of the noisy -> Optional[str] and such you need to annotate Lutris as it is; I don't think he'd like Union[str, dict] either, for that matter, when Lutris does that sort of thing (not often!)

He favors not returning None so much, rather than type annotations that describe when we do that.

Preferably, functions should return consistently typed data, or raise exceptions if they can't, as I understand it.

danieljohnson2 avatar Jan 15 '24 22:01 danieljohnson2

Difference between

  • broken CI - https://github.com/lutris/lutris/actions/runs/7526224000/job/20483961133
  • good CI - https://github.com/lutris/lutris/actions/runs/7532481402/job/20503181065

is running additionally make dev, make req-python

req-python:
	pip3 install PyYAML lxml requests Pillow setproctitle python-magic distro dbus-python types-requests \
	 types-PyYAML evdev PyGObject pypresence protobuf moddb

dev:
	pip3 install ruff==0.1.13 mypy==1.8.0 mypy-baseline nose2

and removing most of installed system packages

  sudo apt install python3-yaml python3-requests python3-pil python3-gi python3-gi-cairo \
    gir1.2-gtk-3.0 gir1.2-gnomedesktop-3.0 gir1.2-webkit2-4.0 \
    gir1.2-notify-0.7 psmisc cabextract unzip p7zip curl fluid-soundfont-gs \
    x11-xserver-utils python3-evdev libgirepository1.0-dev \
    python3-setproctitle python3-distro

become

  sudo apt-get install libdbus-1-dev pkg-config libgirepository1.0-dev

qarmin avatar Jan 16 '24 07:01 qarmin

Ah, thank you, that's got it. I was missing pip3 install requests.

It looks like you've done your homework. I dunno how @strycore feels about changing our build infrastucture so much, but this makes me feel better about the prospect.

danieljohnson2 avatar Jan 16 '24 09:01 danieljohnson2

But there are no changes in the building process.

I've only replaced some tools with ruff and added a req-python step to make it easier to install python dependencies for mypy and ruff in CI, but it is not used by any other app part

qarmin avatar Jan 16 '24 14:01 qarmin