Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make codebase work with basic mypy usage #1084

Merged
merged 2 commits into from
Aug 4, 2019

Conversation

cschramm
Copy link
Member

@cschramm cschramm commented Aug 4, 2019

mypy -p blueman --ignore-missing-imports reports no findings with these changes (mypy 0.670 and 0.720).

@cschramm cschramm requested a review from infirit August 4, 2019 08:58
Copy link
Contributor

@infirit infirit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All my comments are for variables not having a value assigned where before it would be None. Not assigning a value causes an AttributeError for me, am I missing something?

>>> class Test(object):
	__test__: str

	
>>> Test.__test__
Traceback (most recent call last):
  File "<pyshell#13>", line 1, in <module>
    Test.__test__
AttributeError: type object 'Test' has no attribute '__test__'

Below works

from typing import Optional
class Test(object):
	__test__: Optional[str] = None

__icon__ = None
__priority__ = None
__icon__: str
__priority__: int
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional[str] = None

@@ -16,7 +16,7 @@ class NMConnectionError(Exception):


class NMConnectionBase(object):
conntype = None
conntype: str
Copy link
Contributor

@infirit infirit Aug 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conntype: Optional[str] = None

__description__ = None
__author__ = None
__description__: str
__author__: str
Copy link
Contributor

@infirit infirit Aug 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional[str] = None


__unloadable__ = True
__autoload__ = True

__instance__ = None

__gsettings__ = None
__gsettings__: "GSettings"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__gsettings__: Optional["Gsettings"] = None

instances = []
__plugin_info__ = None
instances: List["ServicePlugin"] = []
__plugin_info__: Tuple[str, str]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__plugin_info__: Optional[Tuple[str, str]] = None


class GSettings(TypedDict):
schema: str
path: None
Copy link
Contributor

@infirit infirit Aug 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

path: Optional[str] = None

@infirit
Copy link
Contributor

infirit commented Aug 4, 2019

Also do you have typehints for PyGobject? I tried [1] but they are incomplete so mypy is reporting lots of problems for me. [2] looks promising but is not merged.

[1] https://github.com/pygobject/pygobject-stubs
[2] pygobject/pgi-docgen#176

@@ -47,7 +49,7 @@ class Base(Gio.DBusProxy, metaclass=BaseMeta):
__name = 'org.bluez'
__bus_type = Gio.BusType.SYSTEM

__gsignals__ = {
__gsignals__: Dict[str, Tuple[Any, GObject.SignalFlags, Tuple]] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first Tuple starts with GObject.SignalFlags not Any, next is None and then a tuple. For the second tuple we know the first is a str and we know what types to expect from dbus properties which we can represent as a Union. So I think it should be the below which works for Base.
Dict[str, Tuple[GObject.SignalFlags, None, Tuple[str, Union[str, int, bool, List[str], Dict[str, GLib.Variant], Dict[int, GLib.Variant]], str]]]].

But not for subclasses that add their own signals so they will need their own type hints.

@cschramm
Copy link
Member Author

cschramm commented Aug 4, 2019

variables not having a value assigned where before it would be None. Not assigning a value causes an AttributeError for me

Where do you get such an exception? The idea is that those properties are not optional, so they cannot be None and are just undefined in the abstract base class but defined in all concrete child classes. The latter might not hold everywhere but I did not run into any issues so far.

Also do you have typehints for PyGobject?

Nope. Not much more than what you know. PyGObject is the main reason for relying on --ignore-missing-imports at the moment. IntelliJ generates some decent stubs but also recommends installing PyGObject-stubs and both aren't perfect.

The inner tuple is actually a tuple of an arbitrary number of GObject.GTypes that are always GObject.TYPE_PYOBJECT and a maximum of three in our case, so a precise type would be `Union[Tuple[()], Tuple[Literal[GObject.TYPE_PYOBJECT]], Tuple[Literal[GObject.TYPE_PYOBJECT], Literal[GObject.TYPE_PYOBJECT]], Tuple[Literal[GObject.TYPE_PYOBJECT], Literal[GObject.TYPE_PYOBJECT], Literal[GObject.TYPE_PYOBJECT]]]`, however as we do not have useful gi stubs no real checks get applied anyway, so we can keep it simple.
@infirit
Copy link
Contributor

infirit commented Aug 4, 2019

Where do you get such an exception? The idea is that those properties are not optional, so they cannot be None and are just undefined in the abstract base class but defined in all concrete child classes. The latter might not hold everywhere but I did not run into any issues so far.

Not in blueman as at the moment all subclasses define them before using. If these are not optional how are you going to enforce the plugin defining them without crashing? Or is the plugin crashing the way 😄.

The Optional for the path in gsettings is really optional so it needs fixing.

@cschramm cschramm merged commit 531da47 into blueman-project:master Aug 4, 2019
@cschramm cschramm deleted the mypy branch August 4, 2019 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants