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

Restcomm_Python_SDK #1

Merged
merged 10 commits into from Jun 23, 2017

Conversation

Projects
None yet
2 participants
@mdsharique
Contributor

mdsharique commented Jun 1, 2017

No description provided.

@deruelle deruelle requested a review from gvagenas Jun 1, 2017

@gvagenas

Hi @mdsharique , thanks for the great work so far.

Keep in mind that I am not a Python expert and you should provide advices on most of the concerns here.

So here are my comments and direction for the next steps:

  1. Base url should not be set to cloud.restcomm.com. the base url should be passed to the application during startup.
  2. You should use the JSON endpoints
  3. Please provide tests based on a mock framework for Restcomm-Connect responses. don't provide automated tests that are using cloud.restcomm.com
  4. Provide tutorial and documentation for the SDK
  5. Can we set the namespace to something like org.restcomm.connect.sdk.python , is this according the Python conventions?

Also, I think we should somehow make the SDK a package that can be installed with pip.

@mdsharique

This comment has been minimized.

Contributor

mdsharique commented Jun 8, 2017

Thank you Sir for your reviews and suggestions. I will soon start working on your suggestions.
--i have to see whether we can set namespace to something like "org.restcomm.connect.sdk.python" or not in python. Will implement if possible
--i have already made SDK a package which can be installed with pip. Will share after making required changes

@gvagenas

This comment has been minimized.

Contributor

gvagenas commented Jun 8, 2017

Great @mdsharique , ping me when ready.
Thanks

@mdsharique

This comment has been minimized.

Contributor

mdsharique commented Jun 15, 2017

@gvagenas

This comment has been minimized.

Contributor

gvagenas commented Jun 22, 2017

@mdsharique , here are my comments:

  1. Please merge README.md and README.rst. Why we need the README.rst?
  2. Provide more details in the README.md:
  • Give a short description of the project (1-2 lines)
  • how to build the project
  • how to run the tests
  • Python version supported
  • Reference any dependencies that might be needed to be added by a developer
  • Provide instructions for where to download Restcomm and point to the restcomm documentation site
  • Provide information how and where developers can open issues and feature requests
  1. Is it possible to have difference folder for main classes and test classes. For example for Java we have main folder that contians the application classes and test that contains the test classes. Is this possible and according to the code style of Python?
  2. Provide instructions for the example class you have there, how to run it, what are the prerequisites etc.

In general, imagine a developer familiar with Python but not with Restcomm, that wants to check source code and use it in his project.

We are almost done and after the above minor details we can merge this PR
Please open issues for the missing features/tasks etc and next PRs will be according to those issues.

Great work so far 👍

@mdsharique

This comment has been minimized.

Contributor

mdsharique commented Jun 22, 2017

@gvagenas As you have asked, I have done all the minor Changes

@gvagenas

This comment has been minimized.

Contributor

gvagenas commented Jun 23, 2017

Hi @mdsharique great work, I will proceed to the merge now.
Please open issues for the missing features and next tasks and create PR according to the issue you will be working.
Also the tests you provided are not proper and are more examples than unit tests but I will send an email about this, for now we can keep them as is

@gvagenas gvagenas merged commit 2e488cd into RestComm:master Jun 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment