Typing/Linting/"Quality Gates" in ProtonUp-Qt

[sonic2kk]

I didn't open this as an issue as this is more of a general question. There may be more pressing things, of course, but I think opening Discussions is more for general chat around a broad idea rather than a more focused "enhancement" issue.

ProtonUp-Qt uses Python's type-hinting a bit sporadically. And because the project has been around for a little while and targetted a few different Python versions, some of the codebase is still using the older typing syntax with from typing import Dict, List etc. Now, we target Python 3.10, and I think we discussed before about moving the codebase over gradually to use the newer syntax where we can (i.e. using foo: list instead of foo: List. However, this comes with a challenge. Many linters when you type with a list will want you to explicitly type the list, dict, etc. But on the other hand, this syntax is deprecated in Python 3.9. I did open one PR to resolve one instance of this (#416).

I have recently began using a linter called basedPyright, which is very strict by default in particular with typing (to the point of wanting explicitly typed dicts). It can however be configured to be less-strict, i.e. we can disable certain warnings or reduce some down to warnings in cases where we think certain warnings can be useful in some circumstances but are too much effort to work around in others without specific code changes. Keeping these as warnings means we can still keep track of them as "tech debt" that could be resolved as part of a broader change.

Perhaps we could come up with a configuration for what we want to and don't want to lint?

Type hinting in Python can be a bit of a fierce topic, and Python doesn't exactly enforce it (by design iirc, which is part of the fierce debate). But it's not the only linting warning. Certain PEP protocols can be very strict by default such as maximum line length (some projects set a standard inspired by this but not the default which is very restrictive).

---

So I think the overall purpose of this discussion is to discuss how we want to handle typing and the overall idea of "quality gates" going forward.

• When/how do we want to handle removing deprecated types? I opened one PR but should we do it per-file? Sometimes the changes will be straightforward but some linters will want more explicit typing, i.e. they'll want us to define list[str] instead of list. We already do this to some degree but not consistently. This also leads to another issue:
• How strict do we want to be with typing and linters? Maybe how we plan to integrate tests into the project could determine this. Perhaps we don't want basedPyright to be our baseline but want to use a different checker for the codebase?
  • If we do decide to enforce some "quality gates" it can go beyond typing, to enforcing certain PEPs or reducing some things to warnings to still track them, but not enforce them yet as it may be non-trivial.
  • We have some choices on how we want to enforce PEP style checks in terms of which dependencies to use, such as pycodestyle. Which type checker and style checker we use, and how strict we want to make them, is certainly a point of discussion.
• Depending on where we land, structuring the PRs is something to think about. Some changes might just be drop-ins, replacing List with list, but typing them might be tricky and create a larger diff. There may also be interconnected dependencies, for example typing a variable that gets assigned a return value of a method, when the method itself is not yet typed.

Some of the typing/linting changes can be done ad-hoc in a PR but some, if we decide for example to enforce partially strict type hinting, could require significant changes that might pollute a PRs diff, and those might be better served in their own PR.

If we decide to go forward with this, once things are ironed out and discussed and we have some project guidelines in place, we could put this into a CONTRIBUTING.md document.

---

No, this isn't a massively important topic. But ProtonUp-Qt is in a nice spot. I have experience with a project where there is limited opportunity for structure in the codebase and that has partially inspired my proposal for tests and for this. I also partially want to reduce the noise of basedPyright without simply tuning everything to my preferences, I figured if we could get a standard for the project that would work nicely.

And, if this is something you don't think is very valuable to the project, we can close this or shelve it for now if you think it might be a good discussion in the future, but not right now.

[sonic2kk]

It should also be noted that some warnings would be impossible to mitigate right now to my knowledge. For example accessing properties that we load from the UI files. There will always be warnings that self.ui.myAmazingButton doesn't exist.

[sonic2kk]

I created an example of what a fully type-hinted file might look like. This resolved all the warnings basedPyright was throwing at me: sonic2kk@e5f79d7

This doesn't have any PEP or line-length flags though. I imagine if we start including those the structure of the file to begin to look very different.

[DavidoTek]

Interesting discussion. I once ran mypy over ProtonUp-Qt and the number of warnings/errors were scary 😄

I opened one PR but should we do it per-file?
There may also be interconnected dependencies, for example typing a variable that gets assigned a return value of a method, when the method itself is not yet typed

I feel like doing it per "type of change" makes sense if applicable. Otherwise per-file is okay I guess.
Yeah, probably makes sense to do it in pairs, when typing a variable to also type the return type.

Perhaps we could come up with a configuration for what we want to and don't want to lint?
How strict do we want to be with typing and linters?
And, if this is something you don't think is very valuable to the project, we can close this or shelve it for now if you think it might be a good discussion in the future, but not right now.

I think it depends a bit. There are some places where the types can't break much.
But in other places, wrong typing can cause lots of errors (take my_var: str = None and dictionaries for example). I would concentrate on those first.

If we decide to go forward with this, once things are ironed out and discussed and we have some project guidelines in place, we could put this into a CONTRIBUTING.md document.
Maybe how we plan to integrate tests into the project could determine this. Perhaps we don't want basedPyright to be our baseline but want to use a different checker for the codebase?

I guess both putting it into CONTRIBUTING.md and adding tests makes sense in the long term.

It should also be noted that some warnings would be impossible to mitigate right now to my knowledge. For example accessing properties that we load from the UI files. There will always be warnings that self.ui.myAmazingButton doesn't exist.

Yes, that is something we need to consider. We cannot type objects with dynamic attributes...

I created an example of what a fully type-hinted file might look like. This resolved all the warnings basedPyright was throwing at me: sonic2kk/ProtonUp-Qt@e5f79d7

POSSIBLE_STEAM_INSTALL_LOCATIONS: list[dict[str, str]] = [
{
    'install_dir': f'{_STEAM_ROOT}/compatibilitytools.d/',
    'display_name': 'Steam',
    'launcher': 'steam',
...

That is a good example for the "dictionary typing". dict[str, str] isn't very descriptive of what is actually in the dictionary. The only thing we can assume is that dict.get(x) will return either str or None. Ideally, we would replace all of those will well typed dataclasses.

This doesn't have any PEP or line-length flags though. I imagine if we start including those the structure of the file to begin to look very different.

That's another thing. I wonder if we should enforce tools like pep8 or black.
But that also means we need to apply it once and have a really really massive PR that changes almost every line. That will break all open PRs and security people also wouldn't like it 😄

[sonic2kk]

Interesting discussion. I once ran mypy over ProtonUp-Qt and the number of warnings/errors were scary 😄

Just a sea of warnings and errors everywhere 😄 Making changes just to appease a linter is not itself beneficial, and we can turn off warnings probably even per-file where we think it's too excessive. It can act as a guide though for if we want to think about the structure of the codebase going forward.

I think it depends a bit. There are some places where the types can't break much.
But in other places, wrong typing can cause lots of errors (take my_var: str = None and dictionaries for example). I would concentrate on those first.

Yes I think this makes sense. We could implement type hinting first in the places that are likely to trip us up so they can act as guardrails.

What do you think about making a PR to remove the deprecated types imported from typing like List and Dict? I won't go through and explicitly type them, i.e. List wouldn't become List[str] unless it's pretty clear-cut and straightforward. The reason I would ask to "prioritise" these as well is to remove a barrier to upgrade Python versions in future. We're on 3.10 at the moment, the next version for our AppImage base is probably going to be 3.12, and these types won't be removed until 3.14.

But the AUR packages (unsure about the Flatpak Python version) are using the system Python version. Come the next Python version if these are potentially removed, we might end up with breakages. So it might be a good idea to prioritise this now as well.

That is a good example for the "dictionary typing". dict[str, str] isn't very descriptive of what is actually in the dictionary. The only thing we can assume is that dict.get(x) will return either str or None. Ideally, we would replace all of those will well typed dataclasses.

I think this is cleaner overall and would be something to look at as well, but could also be a significant refactor overall 😄

But that also means we need to apply it once and have a really really massive PR that changes almost every line. That will break all open PRs and security people also wouldn't like it 😄

We could always do it in stages and steps. We could run them locally and make incremental PRs to improve the codebase, and once we get probably a good 90% of the way there, we could add it as a GitHub Action or something to validate against PRs.

---

Again, this is nothing huge, this is like wishlist-level discussion. Nothing is on fire, nothing needs fixed yesterday, and it could serve as an entry point for any future contributors too.

[sonic2kk]

In #437 I noted that one potential warning we can run into with checkers is when we ignore functions that return a value. This happens sometimes with functions that we created (such as with dbusutil), but also happens in situations where we probably don't care, such as when writing to files - File.write() returns the number of bytes written and, generally speaking, we probably don't care a whole lot about this.

The recommended solution is to assign the result of the call to _, i.e. _ = function_that_returns_a_value().

I think this is a potential case where we might want to keep this, but demote it to a warning / equivalent rather than an angry error. Something we can encourage to be displayed but only handle it on a case-by-case basis. It might become overly noisy to use _ = each time, although we can certainly do that.