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

Change matrix component to use matrix-nio instead of matrix_client #72797

Merged
merged 64 commits into from
Sep 2, 2023

Conversation

PaarthShah
Copy link
Contributor

@PaarthShah PaarthShah commented Jun 1, 2022

Proposed change

This PR introduces a new dependency, matrix-nio, as a replacement for the obsolete matrix_client.

  • Allows for the use of asyncio operations to greatly speed up operations involving network I/O.
  • Will allow for use of E2EE, and other important features of Matrix that have become common in the last few years.
  • Because the developers of the old dependency, matrix-python-sdk, specifically recommend the use of matrix-nio as "both more featureful and more actively maintained".

Furthermore, this PR aimed to validate the dependency upgrade by introducing new unit tests (was previously NOT tested) and python type hinting. Currently, I've achieved 97% code coverage of the component code.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

THIS SHOULD NOT BE A BREAKING CHANGE.

Every effort was taken to maintain (and test!) all previous functionality, with the exception of exact code logic (and resulting logging statements) used to achieve it.

I have deliberately not done any refactoring of existing code except as to take advantage of newer python syntax or to support best testing practices. If such cleanup is desired, I can perform it in this PR, or in a separate PR.

I have also deliberately not implemented Config Flow, in order to keep PR size down. Its implementation, along with other matrix-specific features, are in my future plans.

I am happy to explain any apparent functionality difference, or fix it if it is actually incorrectly replicating existing behavior.

Though I have checked multiple boxes under Type of change, this was done solely in light of how the matrix component currently does not have any unit tests, and I thought it would be prudent/appropriate to include such tests while making such a reaching change.

If, before/during review it is requested to split the dependency upgrade and addition of unit tests into separate PRs, I am happy to do so.

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@homeassistant
Copy link
Contributor

Hi @PaarthShah,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@probot-home-assistant
Copy link

Hey there @tinloaf, mind taking a look at this pull request as it has been labeled with an integration (matrix) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@PaarthShah PaarthShah changed the title Change matrix component to use matrix-nio instead of matrix_client Change matrix component to use matrix-nio instead of matrix_client, and add unit tests, strict typing. Jun 1, 2022
@MartinHjelmare MartinHjelmare changed the title Change matrix component to use matrix-nio instead of matrix_client, and add unit tests, strict typing. Change matrix component to use matrix-nio instead of matrix_client, and add unit tests, strict typing Jun 1, 2022
@PaarthShah
Copy link
Contributor Author

PaarthShah commented Jun 9, 2022

Hey @arychj! Apologies for pinging you directly, but I've seen that you've also done some recent work with the matrix component such as #69951 (which is a nice change and thankfully doesn't look like it'll be a major conflict to resolve, whenever it gets merged).

I was wondering if you/@Kane610 (since I saw they also reviewed matrix-related changes) could take a look at this PR and offer any advice for getting this merged! Would be happy to talk on another platform as desired. :)

@PaarthShah PaarthShah marked this pull request as ready for review August 24, 2023 10:03
@home-assistant home-assistant bot requested a review from edenhaus August 24, 2023 10:03
Copy link
Contributor

@edenhaus edenhaus left a comment

Choose a reason for hiding this comment

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

Can you please verify that past conversations are resolved? Currently, there are still four open.

tests/components/matrix/test_login.py Outdated Show resolved Hide resolved
tests/components/matrix/test_login.py Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft August 28, 2023 07:59
@PaarthShah
Copy link
Contributor Author

Can you please verify that past conversations are resolved? Currently, there are still four open.

@edenhaus I've marked each of the past 4 (and your 2 most-recent) conversations as resolved: they were mainly along the lines of general discussion, and since-resolved comments from [@]balloob from before when I knew I should've just marked them resolved on my own :^)

@edenhaus
Copy link
Contributor

edenhaus commented Sep 1, 2023

You don't need to ping me on the process :) When you have addressed all changes, please mark the PR as ready for review, and the bot will ask for a review from me.

@PaarthShah PaarthShah marked this pull request as ready for review September 1, 2023 07:25
@PaarthShah
Copy link
Contributor Author

Apologies! Since it was a comment that couldn't itself be marked as resolved I wasn't sure of the etiquette 😅

Copy link
Contributor

@edenhaus edenhaus left a comment

Choose a reason for hiding this comment

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

Some minor changes to improve the readability of the test.

tests/components/matrix/test_login.py Outdated Show resolved Hide resolved
tests/components/matrix/test_login.py Outdated Show resolved Hide resolved
tests/components/matrix/test_login.py Outdated Show resolved Hide resolved
tests/components/matrix/test_login.py Outdated Show resolved Hide resolved
tests/components/matrix/test_login.py Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft September 1, 2023 09:44
@edenhaus
Copy link
Contributor

edenhaus commented Sep 1, 2023

Sanity check 0.21.2 ✔️

@PaarthShah During the check, i found the following line https://github.com/poljar/matrix-nio/blob/0.21.2/nio/api.py#L51-L52. Probably a historic left over

tests/components/matrix/test_login.py Outdated Show resolved Hide resolved
tests/components/matrix/test_login.py Outdated Show resolved Hide resolved
@PaarthShah
Copy link
Contributor Author

Sanity check 0.21.2 ✔️

@PaarthShah During the check, i found the following line https://github.com/poljar/matrix-nio/blob/0.21.2/nio/api.py#L51-L52. Probably a historic left over

Oof yeah, I've made a long laundry list of cleanup tasks for matrix-nio since becoming a contributor for it: that's definitely a good find!

@PaarthShah PaarthShah marked this pull request as ready for review September 1, 2023 17:20
Copy link
Contributor

@edenhaus edenhaus left a comment

Choose a reason for hiding this comment

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

Thanks @PaarthShah 👍

@edenhaus edenhaus changed the title Change matrix component to use matrix-nio instead of matrix_client, and add unit tests, strict typing Change matrix component to use matrix-nio instead of matrix_client Sep 2, 2023
@edenhaus edenhaus merged commit 4d3b978 into home-assistant:dev Sep 2, 2023
34 checks passed
@herostrat herostrat mentioned this pull request Sep 2, 2023
22 tasks
@PaarthShah PaarthShah deleted the matrix-nio branch September 2, 2023 21:24
@github-actions github-actions bot locked and limited conversation to collaborators Sep 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.