Skip to content

Move main api methods into transport-agnostic class #128

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

Closed
wants to merge 3 commits into from

Conversation

non-Jedi
Copy link
Collaborator

@non-Jedi non-Jedi commented May 6, 2017

Closes #37

I was going to start working on converting the tests over to not using requests, but I'm not sure how this we should do that keeping in mind @pik's #125. I actually kind of rushed to get this one done after seeing that because I want us to have the right architecture before we really start building out our testing.

@matrixbot
Copy link
Member

Can one of the admins verify this patch?

This allows easy implementation of alternative transports (e.g.
websockets) in the future by subclassing MatrixApi. It also allows
testing of the api without mocking requests.

Signed-off-by: Adam Beckmeyer <[email protected]>
@pik
Copy link
Contributor

pik commented May 6, 2017

I don't understand the logic behind this, requests is used in one place, we can replace it with urllib2/3 without much pain (as can anyone else needing this by patching _send) or forking the sdk, imho let's not make using the sdk more complicated than it is.

@non-Jedi
Copy link
Collaborator Author

non-Jedi commented May 9, 2017

Some of the work I had meant to include in this PR isn't there. :/

And I'm actually slightly rethinking the way I want to do this (partially because of @pik's comment). So please hold off on looking at this until I get a chance to push another commit or two.

@non-Jedi
Copy link
Collaborator Author

non-Jedi commented Sep 5, 2017

closing this until I get time to work on it again

@non-Jedi non-Jedi closed this Sep 5, 2017
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.

3 participants