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.

Sign up for GitHub

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

Jump to bottom

Matrix connector #731

Merged
merged 35 commits into from
Jan 16, 2019
Merged

Matrix connector #731

merged 35 commits into from
Jan 16, 2019

Conversation

SolarDrew
Copy link
Contributor

@SolarDrew SolarDrew commented Nov 8, 2018 •
edited
Loading

Port Matrix connector into core

Start of efforts to move the Matrix connector in the core library

Fixes #662

  • Make a new submodule directory in opsdroid.connector and copy the connector code over.
  • Update the requirements.txt with any dependencies from the connector if necessary.
  • Write tests for the connector.
  • Copy the relevant information from the connector README.md into a new documentation page.
  • Add the new page to the mkdocs.yml.
  • Add to the list of connectors.
  • Add a deprecation notice to the old connector. (Pending in Add deprecation message to readme connector-matrix#42)

Status

READY

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

  • Existing tests pass
  • New tests have been added and also pass

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@SolarDrew
Copy link
Contributor Author

Note that I've ported the matrix connector across complete with its commit history, which I'm happy to purge if we don't want it cluttering up the place.

@FabioRosado
Copy link
Member

Thanks for working with this, it's alright with all these commit because we always squash while merging the PRs

@jacobtomlinson
Copy link
Member

Thanks for this! I see you've ticked the documentation related tasks but I can't see them in the diff.

@SolarDrew
Copy link
Contributor Author

Oh, sorry, that probably means I'm a moron and forgot to commit them. I'll try and fix them and untick those tasks in the meantime.

@codecov
Copy link

codecov bot commented Nov 9, 2018 •
edited
Loading

Codecov Report

Merging #731 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #731    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files          30     33     +3     
  Lines        1934   2046   +112     
======================================
+ Hits         1934   2046   +112
Impacted Files Coverage Δ
opsdroid/connector/matrix/html_cleaner.py 100% <100%> (ø)
opsdroid/connector/matrix/connector.py 100% <100%> (ø)
opsdroid/connector/matrix/__init__.py 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c19a4c9...f0bfa71. Read the comment docs.

SolarDrew
SolarDrew commented Nov 9, 2018
View reviewed changes
requirements.txt Outdated
@@ -12,3 +12,5 @@ websockets==7.0
appdirs==1.4.3
multidict==4.4.2
motor==2.0.0
git+https://github.com/tinloaf/matrix-python-sdk@groups
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Cadair is there a more stable position for this now that I can point to? Alternatively, @jacobtomlinson is there a way of specifying a repo as a requirement that doesn't break the tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be my async api package now? We don't need to rely on the groups branch for this.

Copy link
Member

Choose a reason for hiding this comment

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

Sadly we use the requirements.txt file in the setup.py too for dependencies, so a git style requirement is not supported. I recommend using (or publishing your own) package on pypi/conda-forge.

Copy link
Contributor

Choose a reason for hiding this comment

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

this line should just be matrix-api-async

@stale
Copy link

stale bot commented Nov 26, 2018

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 26, 2018
@jacobtomlinson
Copy link
Member

@Cadair @SolarDrew any chance you intend to pick this up again?

@stale stale bot removed the stale label Nov 26, 2018
@SolarDrew
Copy link
Contributor Author

Well I definitely intend to... I'll try and do some more work on it this week.

@stale stale bot removed the stale label Nov 26, 2018
@SolarDrew
Copy link
Contributor Author

I force pushed to remove all the connector git history because I'm awful and you were going to squash it anyway. Tests still ongoing but all the code should be mostly in place now.

@stale
Copy link

stale bot commented Dec 23, 2018

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 23, 2018
@FabioRosado
Copy link
Member

Not stale, bad bot 😄

@stale stale bot removed the stale label Dec 24, 2018
@jacobtomlinson jacobtomlinson mentioned this pull request Dec 30, 2018
Merged
4 tasks
@stale
Copy link

stale bot commented Jan 7, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 7, 2019
@jacobtomlinson
Copy link
Member

Happy new year everyone! Any chance you can find some time to work on this?

@stale stale bot removed the stale label Jan 10, 2019
@SolarDrew
Copy link
Contributor Author

Happy new year everyone! Any chance you can find some time to work on this?

Happy new year :) Yes, apologies for the slow progress on this. @Cadair is helping me out with some of the testing stuff, which was where I'd got stuck, so there should start to be some progress on this pretty soon, hopefully.

@Cadair Cadair force-pushed the matrix-connector branch 2 times, most recently from 5b758b0 to ba6e033 Compare January 16, 2019 11:15
@SolarDrew SolarDrew mentioned this pull request Jan 16, 2019
Closed
@SolarDrew SolarDrew changed the title [WIP] Matrix connector Matrix connector Jan 16, 2019
@FabioRosado
Copy link
Member

🎉 This is awesome! Thanks for working on this I am merging this now 👍

@FabioRosado FabioRosado merged commit 8c400e7 into opsdroid:master Jan 16, 2019
@SolarDrew SolarDrew mentioned this pull request Jan 16, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cadair Cadair Cadair left review comments

@jacobtomlinson jacobtomlinson jacobtomlinson left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move Matrix connector into core
4 participants
@SolarDrew @FabioRosado @jacobtomlinson @Cadair