Skip to content

next merge: Helper NetworkManagerSettings.get_connections_by_id()#8

Merged
igo95862 merged 1 commit intopython-sdbus:masterfrom
bernhardkaindl:helpers/get_connections_by_id
Apr 2, 2022
Merged

next merge: Helper NetworkManagerSettings.get_connections_by_id()#8
igo95862 merged 1 commit intopython-sdbus:masterfrom
bernhardkaindl:helpers/get_connections_by_id

Conversation

@bernhardkaindl
Copy link

@bernhardkaindl bernhardkaindl commented Mar 29, 2022

In PR #6, where I initially added a connection_by_id() function for the purpose of that example, I didn't account for the fact that NetworkManager can have many connections with the same id. Only the uuid is guaranteed to be unique.

For a helper method like you suggested to be provided by python-sdbus-networkmanager, we should be able to potentially return multiple connections, so I'd return a list of them.

This PR provides this helper in the NetworkManagerSettings class, alongside the get_connection_by_uuid which is provided as part of the DBus interface by this class.

An example use in examples which add connections can look like this:

    if NetworkManagerSettings().get_connections_by_id(args.conn_id):
        print(f'Connections using ID "{args.conn_id}" exist, remove them:')
        print(f'Run: nmcli connection delete "{args.conn_id}"')
        return

@bernhardkaindl
Copy link
Author

@igo95862: Rebased to the merge commit of #7, this could be merged next!

@bernhardkaindl bernhardkaindl changed the title Add helper method NetworkManagerSettings().get_connections_by_id() next merge: Helper NetworkManagerSettings.get_connections_by_id() Apr 2, 2022
@bernhardkaindl bernhardkaindl requested a review from igo95862 April 2, 2022 18:08
@bernhardkaindl
Copy link
Author

bernhardkaindl commented Apr 2, 2022

This is the new blocking version with the improved docstring and the "annotated" version of reading the value of id:

    def get_connections_by_id(self, connection_id: str) -> List[str]:
        """Helper method to get a list of connection profile paths
        which use the given connection identifier.

        :param str connection_id: The connection identifier of the connections,
        e.g. "Ethernet connection 1"
        :return: List of connection profile paths using the given identifier.
        """
        connection_paths_with_matching_id = []
        value = 1
        for connection_path in self.connections:
            s = NetworkConnectionSettings(connection_path)
            if s.get_settings()["connection"]["id"][value] == connection_id:
                connection_paths_with_matching_id.append(connection_path)
        return connection_paths_with_matching_id

@igo95862 The asyncio version is changed likewise. I think this addresses your remarks so far.

@igo95862
Copy link
Collaborator

igo95862 commented Apr 2, 2022

["connection"]["id"][1] was a better option.

It is my mistake that I forgot about the variant type.

@igo95862
Copy link
Collaborator

igo95862 commented Apr 2, 2022

I think I will extend the tuple class that Variant returns. Add something like unwrap as string and etc...

@igo95862
Copy link
Collaborator

igo95862 commented Apr 2, 2022

I think the entire settings dictionary can be replicated with some dataclass. This would allow to document all settings that the objects can have and allow typing the settings. Every method that returns or takes a settings dictionary would have a sibling helper method that would return the dataclass.

@bernhardkaindl
Copy link
Author

["connection"]["id"][1] was a better option.

pushed.

@bernhardkaindl
Copy link
Author

bernhardkaindl commented Apr 2, 2022

Thanks for the approval. Since I'm not invited as a developer, of course only you can do the merge.

BTW, in these discussions, there are some nice approaches on creating classes which could be a base for providing a class-namespace on top of nested dicts with lists and tuples:

https://stackoverflow.com/questions/13520421/recursive-dotdict
https://stackoverflow.com/questions/3031219/recursively-access-dict-via-attributes-as-well-as-index-access

I think one of the examples (I hope I can find it again) which I saw even said that writing to the class writes even to the underlying dict.

That feature (writing to the underlying dict on changes, or exporting to a nested dict again) would be essential for using such dict in an update_connection example.

This is a pip package which can do that, but I'm not sure if it can be customized to use the Variant tuples and turn them into types and back to the variant tuples:
https://github.com/Infinidat/munch

@igo95862 igo95862 merged commit b077a60 into python-sdbus:master Apr 2, 2022
@igo95862
Copy link
Collaborator

igo95862 commented Apr 2, 2022

BTW, in these discussions, there are some nice approaches on creating classes which could be a base for providing a class-namespace on top of nested dicts with lists and tuples:

I would go with dataclass from standard library. The typing information can be implemented with it.

Since there is upstream documentation on all values for all settings dictionaries I would implement them tomorrow.

Thank you again for working on pull requests.

@bernhardkaindl bernhardkaindl deleted the helpers/get_connections_by_id branch April 3, 2022 12:22
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