python-sdbus / python-sdbus-networkmanager

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

Use the new enum ConnectionType and support using it without .value #34

Closed bernhardkaindl closed 2 years ago

bernhardkaindl commented 2 years ago

@igo95862: This uses the new enum ConnectionType and improves it to not require to use .value:

The article https://www.cosmicpython.com/blog/2020-10-27-i-hate-enums.html descibe shortcomings of enums with str:

class ConnectionType(str, Enum):
     ADSL = "adsl"
     BLUETOOTH = "bluetooth"
     ...

The blog shows need to use it using .value because theenumitself does not provide a properdef str()`.

    if connection.connection_type == ConnectionType.WIFI.value:
         print("SSID:", profile.wireless.ssid.decode())

This gist provided with the blog demonstrates and tests providing def __str__() to improve this: https://gist.github.com/hjwp/405f04802ea558f042728ec5edbb4e62

By providing it like described in the blog article, the issue (danger forgetting to add .value) is solved:

-    if connection.connection_type == ConnectionType.WIFI.value:
+    if connection.connection_type == ConnectionType.WIFI:
         print("SSID:", profile.wireless.ssid.decode())
igo95862 commented 2 years ago

By providing it like described in the blog article, the issue (danger forgetting to add .value) is solved:

I am pretty sure that the only difference with the solution in the article is with calling str() method.

This already works: (on master)

assert '802-11-wireless' == sdbus_async.networkmanager.ConnectionType.WIFI
bernhardkaindl commented 2 years ago

Thanks, works indeed. When removing .value in examples/async/add-wifi-psk-connection-async.py there is a difference:

--- a/examples/async/add-wifi-psk-connection-async.py
+++ b/examples/async/add-wifi-psk-connection-async.py
@@ -87,7 +87,7 @@ async def add_wifi_psk_connection_async(args: Namespace) -> str:
         connection=ConnectionSettings(
             connection_id=args.conn_id,
             uuid=str(args.uuid),
-            connection_type=ConnectionType.WIFI.value,
+            connection_type=ConnectionType.WIFI,
             autoconnect=bool(hasattr(args, "auto") and args.auto),
         ),
         ipv4=Ipv4Settings(method="auto"),

This is the output when it runs:

{'connection': {'id': 'MyConnectionExample',
                'type': <ConnectionType.WIFI: '802-11-wireless'>,
                'uuid': '9bc82eb8-e34b-49a8-8997-f030c0383a54'

It's not good I think, but the example still creates the connection profile, so I can't say that it does not work. Ok with me.

igo95862 commented 2 years ago

It's not good I think, but the example still creates the connection profile, so I can't say that it does not work. Ok with me.

It should still work because <ConnectionType.WIFI: '802-11-wireless'> is a subclass of str. When ConnectionType is encoded to D-Bus it will become regular string.

There were some issues with enums in python-sdbus versions before 0.10.0 but I should have fixed all of them.

I have a unittest to test the str enum encoding:

https://github.com/python-sdbus/python-sdbus/blob/e8dcaa0d71b44637425630446853b216a47072bd/test/test_read_write_dbus_types.py#L418

bernhardkaindl commented 2 years ago

Thanks, closing.

bernhardkaindl commented 2 years ago

@igo95862, could you have a look and comment on this:

I have this code where device.type_name is a string containing the device type as a string:

         profile_settings = {
             "connection": {
                "type": str(getattr(ConnectionType, device.type_name.upper())),
             }
         }

When I print this dictionary, I get:

{'connection': {'type': 'ConnectionType.MODEM'}

That means I'd have to use .value to get the string in this use, and when we have the __str__ method, one could not fall into this trap.

igo95862 commented 2 years ago

Do you actually need to call str()

profile_settings = {
             "connection": {
                "type": getattr(ConnectionType, device.type_name.upper()),
             }
         }

The issue I can see is that getattr is opaque to mypy but that can be fixed by getting the string outside the dictionary literal and then having a assert isinstance.

bernhardkaindl commented 2 years ago

Thanks! Indeed not.

The issue I can see is that getattr is opaque to mypy but that can be fixed by getting the string outside the dictionary comprehension and then having a assert isinstance.

I don't know enough about python/mypy to be sure to understand what you are saying at the moment and I don't how that would look like as code.

What surprised me the most is that str() gets the name of the enum while without it, I get the .value of the enum, which is quite counter-intuitive to me, but so be it.:

A after serializing to JSON and back to a dictionary, indeed I have 'type': 'gsm' on the other end of the data transfer.

I'll use it like this for now because I actually now like how the python library rich highlights the ConnectionType.MODEM string in violet color which helps me to see the enum quickly in the highlighted, very large dictionary:

... {'type': <ConnectionType.MODEM: 'gsm', ...}, ...
igo95862 commented 2 years ago

Good to hear!

I don't know enough about python/mypy to be sure to understand what you are saying at the moment and I don't how that would look like as code.

Basically when getattr is called mypy does not know what the returned type would be. (it would be Any)

You can force the type back to whatever is required with assert isinstance

connection_type = getattr(ConnectionType, device.type_name.upper())
assert isinstance(connection_type, str)

profile_settings = {
             "connection": {
                "type": connection_type,
             }
         }

But it is only relevant if you need strict typing.