python-sdbus / python-sdbus-networkmanager

python-sdbus binds for NetworkManager
GNU Lesser General Public License v2.1
30 stars 6 forks source link

Many updates for the settings-helper branch with examples #16

Closed bernhardkaindl closed 2 years ago

bernhardkaindl commented 2 years ago
bernhardkaindl commented 2 years ago

There is one major thing I don't like, which I guess is not possible to solve while still using dataclass as base class:

Say I want to update a network connection and want to change e.g. the WiFi PSK like so:

    profile.wifi_security.psk = args.password

This triggers this mypy error:

error: Item "None" of "Optional[WifiSecuritySettings]" has no attribute "psk"

The reason for this that the dataclass-based approach (has to use/uses) Optional[WifiSecuritySettings] to add "802-11-wifi-security" and all the other settings domains which are only present for specific connection types.

My idea for fixing this would be to create specific ConnectionProfile top-level classes for each type of connection, and in .from_dbus() of a master class (which would be used when converting from dbus internally as well) check what

 connection_profile["connection"]["type"]

indicates and then create the needed class for the given connection_type. To you see other possible ways to fix this issue?

PS: My Prodict and MetaDict-based approaches does not have this issue.

But it seems, this would be solvable using different ConnectionProfile classes like ConnectionProfileWireless, ConnectionProfileEthernet, etc.

Do you agree that this would be the way forward to fix that?

igo95862 commented 2 years ago

Say I want to update a network connection and want to change e.g. the WiFi PSK like so:

You can insert an assert so that mypy knows:

assert profile.wifi_security is not None
igo95862 commented 2 years ago

I will review this on weekend. A bit busy on workdays.

bernhardkaindl commented 2 years ago

It got a lot of review feedback, thanks! The reviews in this PR are addressed or merged into #14, so this PR is not needed anymore.