search

openzim / youtube

Create a ZIM file from a Youtube channel/username/playlist
GNU General Public License v3.0
44 stars 26 forks source link

Use temporary build_dir #102

Closed satyamtg closed 4 years ago

satyamtg commented 4 years ago

This fixes #101 by using a temporary directory as build dir by default, and passing --output directly to youtube2zim in playlists mode. Also, a new option --build-dir is introduced for custom build dir location.

satyamtg commented 4 years ago

Thanks @rgaudin for reviewing the PR so quickly and extensively. Coming to points -

Understand that when expecting a minor change, seeing many changes, especially touching non-related code is misleading

Yup. I'm sorry for that. I shoul've mentioned exact changes and their reasons. My bad. I'll take care of this from now on.

I think the issue wasn't clear enough that whatever the --build-dir, we'd still use a temp subfolder inside to be sure it's really temporary. This means that it can't be reusable and that --keep is for debugging only.

Yup. I agree to this. It's a much better thing than what I did. I have seen the changes that you did and they seem amazing. Just I have one question (inline comment)

Coming to your questions -

playlists entrypoint to check that variables are used (why?)

Since we are creating multiple ZIMs, I felt that since we were now having ZIMs directly in root of output_dir instead of within subfolders, it becomes that they have unique filenames, that's why I implemented a check of variables so that we ensure unique names. (Should've added check for --zim-file arg too). So, basically that was the idea behind it. I understand that it's not so necessary now.

--name passing from playlists mode to youtube2zim (why?)

We get the name from the JSON, if not that, we get the self.playlists_name or else None. So, we already have checked the self.playlists_name and I feel setting up again may not make sense (I mean I'm not very sure on this). Moreover, if someone put a JSON, but didn't put the 'name' in it, and he hasn't put the --playlists-name argument, then we might end up failing. So I thought failing early with clear message would be good.

I would love to know your take on this. As for ted, I've done your proposed changes locally and once we have this clear, I'll push them. I'll also comprehensively write what things I have changed and why.

rgaudin commented 4 years ago

Thanks, let me try different scenarios on that --name things to be sure.