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

Login feature for Livestation #344

Merged
merged 1 commit into from Apr 3, 2014

Conversation

Projects
None yet
2 participants
@sunaga720
Contributor

sunaga720 commented Apr 3, 2014

Livestation has account system and appearently some streams return different URL when logged in (or perhaps Premium users or not).
I added login feature to livestation plugin.

@chrippa

This comment has been minimized.

Show comment
Hide comment
@chrippa

chrippa Apr 3, 2014

Owner

Looks good, but a couple of improvements can be made:

  • There is no need for a separate requests session, http is already a session.
  • Use the http.json(res) method instead of json.loads.
  • Move the URLs to constant variables instead.
  • The help message for the new options could be more descriptive.
  • Add the new options to docs/cli.rst.
Owner

chrippa commented Apr 3, 2014

Looks good, but a couple of improvements can be made:

  • There is no need for a separate requests session, http is already a session.
  • Use the http.json(res) method instead of json.loads.
  • Move the URLs to constant variables instead.
  • The help message for the new options could be more descriptive.
  • Add the new options to docs/cli.rst.
@sunaga720

This comment has been minimized.

Show comment
Hide comment
@sunaga720

sunaga720 Apr 3, 2014

Contributor

Thanks for comments!

There is no need for a separate requests session, http is already a session.
Use the http.json(res) method instead of json.loads.
Move the URLs to constant variables instead.

I fixed these.

The help message for the new options could be more descriptive.
Add the new options to docs/cli.rst.

I tied fixing these. But I'm not sure they are correct English. (I'm not native)
Would you check them?

Contributor

sunaga720 commented Apr 3, 2014

Thanks for comments!

There is no need for a separate requests session, http is already a session.
Use the http.json(res) method instead of json.loads.
Move the URLs to constant variables instead.

I fixed these.

The help message for the new options could be more descriptive.
Add the new options to docs/cli.rst.

I tied fixing these. But I'm not sure they are correct English. (I'm not native)
Would you check them?

chrippa added a commit that referenced this pull request Apr 3, 2014

Merge pull request #344 from sunaga720/develop
Login feature for Livestation

@chrippa chrippa merged commit 4ae4e23 into chrippa:develop Apr 3, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@chrippa

This comment has been minimized.

Show comment
Hide comment
@chrippa

chrippa Apr 3, 2014

Owner

Looks good, thanks for your contribution!

Owner

chrippa commented Apr 3, 2014

Looks good, thanks for your contribution!

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