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

Create channel api endpoint #2175

Merged
merged 6 commits into from
May 18, 2016
Merged

Create channel api endpoint #2175

merged 6 commits into from
May 18, 2016

Conversation

brussee
Copy link
Member

@brussee brussee commented May 9, 2016 •
edited
Loading

Why are channel names required to be unique? -> Design decision.
And why the silent fail? on channel_manager.py:70 -> Solved.

@tribler-ci
Copy link
Contributor

Can one of the admins verify this patch?

@brussee
Copy link
Member Author

brussee commented May 11, 2016

@whirm ready

@devos50
Copy link
Contributor

devos50 commented May 11, 2016

@brussee you should put READY in front of the title of the PR, otherwise @whirm might miss it ;)

devos50
devos50 reviewed May 11, 2016
View reviewed changes
Tribler/Core/Modules/channel/channel_manager.py
@@ -122,3 +124,7 @@ def get_channel_list(self):
:return: The list of all channel objects.
"""
return self._channel_list


class DuplicateChannelNameError(Exception):
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move this to the exceptions.py file in Tribler/Core.

@devos50
Copy link
Contributor

devos50 commented May 11, 2016

Please also update the documentation with your endpoint description and our new error handling scheme, see here.

@devos50
Copy link
Contributor

devos50 commented May 11, 2016

Other than that, good work 👍

@brussee brussee mentioned this pull request May 13, 2016
Merged
122 tasks
@brussee
Copy link
Member Author

brussee commented May 13, 2016

@devos50: Processed all your comments and added documentation.
@whirm: Ready for your review

@brussee brussee changed the title WIP: create channel api endpoint READY: create channel api endpoint May 13, 2016
@devos50
Copy link
Contributor

devos50 commented May 15, 2016

@brussee I see some files you created in the android directory in this PR. Should they be there?

@brussee
Copy link
Member Author

brussee commented May 16, 2016 •
edited
Loading

@devos50 They are 3 .gitignore files that prevent hundreds of untracked files polluting my git status every time I switch branches from the new Android app to any feature branch from devel.
I don't think they warrant the overhead of a separate pull request and they will end up there eventually.

@brussee brussee changed the title READY: create channel api endpoint WIP: create channel api endpoint May 16, 2016
@brussee
Copy link
Member Author

brussee commented May 16, 2016

The error callback is not called in the HTTP 500 response tests.

@whirm
Copy link

whirm commented May 17, 2016

retest this please

2 similar comments
@whirm
Copy link

whirm commented May 17, 2016

retest this please

@whirm
Copy link

whirm commented May 17, 2016

retest this please

@brussee brussee changed the title WIP: create channel api endpoint READY: create channel api endpoint May 17, 2016
@brussee
Copy link
Member Author

brussee commented May 17, 2016

@whirm Ready for your review

whirm
whirm reviewed May 17, 2016
View reviewed changes
Tribler/Core/Modules/restapi/rest_manager.py
"""
This class gracefully takes care of unhandled exceptions raised during the processing of any request.
"""
defaultContentType = b"text/json"
Copy link

Choose a reason for hiding this comment

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

@lfdversluis @devos50 after this commit is merged, all the setcontent type calls can be removed from all endpoints

@whirm
Copy link

whirm commented May 17, 2016

Also, there's a few things not covered and tests are failing

@whirm whirm changed the title READY: create channel api endpoint WIP: create channel api endpoint May 17, 2016
@brussee brussee force-pushed the my-channel branch 2 times, most recently from 1ef043e to bf056ea Compare May 17, 2016 17:50
@brussee
Copy link
Member Author

brussee commented May 17, 2016

@whirm Ready. 100% test coverage!

@brussee brussee changed the title WIP: create channel api endpoint READY: create channel api endpoint May 17, 2016
@devos50
Copy link
Contributor

devos50 commented May 17, 2016

@whirm I would like to see this one merged before I'm going to refactor the mychannel end points into the channels end points.

whirm
whirm reviewed May 18, 2016
View reviewed changes
android/TriblerApp/.gitignore
@@ -0,0 +1,8 @@
*.iml
Copy link

Choose a reason for hiding this comment

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

Please, merge all the gitignore files with the main one.

@whirm
Copy link

whirm commented May 18, 2016 •
edited
Loading

Looks great, merge the gitignores, rebase and I'll merge

@brussee
Copy link
Member Author

brussee commented May 18, 2016

@whirm done!

@whirm whirm changed the title READY: create channel api endpoint Create channel api endpoint May 18, 2016
@whirm whirm merged commit f2688ed into Tribler:devel May 18, 2016
@brussee brussee deleted the my-channel branch May 18, 2016 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@brussee @tribler-ci @devos50 @whirm