bvanheu / pytoutv

TOU.TV client library and user interfaces written in Python 3
96 stars 23 forks source link

Feature/extra #79

Closed gboudreau closed 7 years ago

gboudreau commented 7 years ago

Rebased on latest master (83aebdb4c871b2be1fba7f73e955fedc7adcae42) and fixed login command that didn't work anymore. Also fixed an issue where it would use the current folder instead of $HOME/.cache/toutv/ folder for cache on Mac.

gboudreau commented 7 years ago

Added another fix where using --url wouldn't work, because the episode page does not contain the emission ID any more; we now need to specify two URLs, for the emission and episode, when using --url with either fetch or info.

gboudreau commented 7 years ago

And finally, I made a first draft of downloading episodes from the Extra section using toutv fetch --url [emission_url] [episode_url]. We still can't load the episode or emission information for Extras, but I think most people just want fetch to work. They can find the URLs to use by browsing ici.tou.tv, and then download the episodes they want from the command line.

simark commented 7 years ago

Woohoo, thanks! I won't have time to look into it tonight, maybe tomorrow night. If somebody else wants to take a look, go ahead.

gboudreau commented 7 years ago

So I was able to make info|search|list|fetch work with emissions and episodes in Extra. Hooray!

Code is a little messy, but it works.

Of note: you will need to delete your .toutv-cache for new emissions & episodes to be loaded from the API.

simark commented 7 years ago

Err, I wanted to comment on the first commit. Is there any way to leave a comment on a particular commit?

simark commented 7 years ago

Pushed your xdg/cache folder commit to master, since it's unrelated and helpful in any case.

simark commented 7 years ago

Instead of requiring the user to enter two URLs (which is a bit tedious), could we solve the problem by having our client do a bit more work? For example, the HTML of an episode contains the show name at least:

            <meta name="rc.emission" content="district-31"/>
            <meta name="rc.codeemission" content="district31"/>

Perhaps we could do another request to look up the show/emission id?

simark commented 7 years ago

Actually, given that an episode url looks like this:

http://ici.tou.tv/district-31/S01E02

We could simply deduce the show URL by keeping the first URL component after the domain... or use the info mentioned in the previous comment and append it to http://ici.tou.tv/. What do you think?

gboudreau commented 7 years ago

@simark To comment on any commit part of a PR, simply click the commit that appears in the PR, and comment on specific lines there, or you can go in the Files changed tab at the top of the PR, and comment on whatever change was made.

gboudreau commented 7 years ago

Could you rebase the current Feature/extra branch on top of the current master, to make this PR easier to read? It should then automatically adjust to only show the changes I made (if not, I'll manually update my branch).

simark commented 7 years ago

Could you rebase the current Feature/extra branch on top of the current master, to make this PR easier to read? It should then automatically adjust to only show the changes I made (if not, I'll manually update my branch).

Ok, I've rebased feature/extra on master and force-pushed.

gboudreau commented 7 years ago

Excellent. Much better now; easier to review.

So I added two commits which allows a user to specify an emission by name or URL for list, info and fetch, and also specify an episode by name or URL for info and fetch. Also, when specifying an episode URL, there is no need to specify an emission URL too; it's all handled automatically.

simark commented 7 years ago

Ah good idea about not having to use --url. It makes it even easier to use.

simark commented 7 years ago

@gboudreau, is it possible that the command toutv list doesn't work? I get:

Unknown exception: <class 'AttributeError'>: 'NoneType' object has no attribute 'startswith'
Traceback (most recent call last):
  File "/home/simark/src/pytoutv/toutvcli/app.py", line 99, in run
    args.func(args)
  File "/home/simark/src/pytoutv/toutvcli/app.py", line 381, in _command_list
    emission, episode = self._get_emission_episode_from_args(args)
  File "/home/simark/src/pytoutv/toutvcli/app.py", line 315, in _get_emission_episode_from_args
    if args.emission.startswith('http'):
AttributeError: 'NoneType' object has no attribute 'startswith'
gboudreau commented 7 years ago

Indeed. Fixed.

simark commented 7 years ago

I don't have much time at the moment to work on this, but I think that it would be a nice goal for the upcoming holidays to merge this branch. The newer API seems much more responsive (i.e. less timeouts) then the old one, and offers more features, such as the extra content, obviously.

We had started a big refactor in the v3-dev branch (including using the new API) and thought it would be the way forward, but given that there's more development in the current branch, I think it's better to go with this one. Thanks for your contribution, it is very much appreciated.

simark commented 7 years ago

@gboudreau, I rebased your branch and added one commit to allow specifying only an URL to the episode (to fetch and info). Basically, doing toutv fetch http://ici.tou.tv/foo/bar is the same as doing toutv fetch foo bar. If you like it, you can reset --hard this PR with this branch.

It is in my repo: https://github.com/simark/pytoutv/tree/feature/extra

gboudreau commented 7 years ago

Failure: toutv search "LES AVENTURES DU PHARMACHIEN" This returns 6 emissions, the correct one twice, and the one emission for each episode! Makes no sense... Also, toutv fetch "LES AVENTURES DU PHARMACHIEN" only tries to download the first episode...

gboudreau commented 7 years ago

Above issue fixed with those last few commits. Side-effect: downloaded files are now always called ShowName.SxxEyy.Épisode.yy.bitratekbps.ts, i.e. Épisode.yy instead of the episode name. This is because the new GET /presentation/search/ endpoint used to load the episodes for a specific show doesn't return the real episode names, for some reason. But using that endpoint is required to ensure we get all the episodes for a show.

simark commented 7 years ago

@gboudreau, I have rebased the branch and pushed it as master. At least now that it's in master, we'll be able to do fixes in more targetted PRs, instead of doing everything in this one.

Thanks a lot!