Replace directory with destination, allow specifying filename (#92) #234
Conversation
…me of downloaded content
| directory_part = self.destination | ||
|
|
||
| # if file name is present in destination | ||
| if(self.target == self.destination): |
There was a problem hiding this comment.
nit: Please remove the brackets. Those are not necessary here.
|
Good start @ujjwalwahi! The changes in the base code looks fine and need some smaller updates. For the tests a bit more work will still be necessary. Therefore please see my inline comments. If you have questions don't hesitate to ask me here or on IRC. Thanks! |
|
Hi @ujjwalwahi. I hope all is fine for you? I would like to know if you can find the time to address my review comments. Or if you have remaining questions. Thanks |
|
Hi @whimboo . I am working on this, little busy with my exams. I updated scrapper.py as per reviews, no issue there. Working with tests. Will deliver it soon. |
|
Good to hear that! But given that you are in exams please concentrate on those for now. This PR can wait. |
…me of downloaded content
|
@whimboo |
| # Don't re-download the file | ||
| if os.path.isfile(os.path.abspath(self.target)): | ||
| self.logger.info("File has already been downloaded: %s" % | ||
| (self.target)) | ||
| return | ||
|
|
||
| directory = os.path.dirname(self.target) | ||
| if directory and not os.path.isdir(directory): |
There was a problem hiding this comment.
We always have a valid directory here. It should never be None. So you can get rid of the first check.
| version=None, | ||
| log_level='ERROR') | ||
| expected_target = os.path.join(self.temp_dir, filename) | ||
| self.assertEqual(scraper.target, expected_target) |
There was a problem hiding this comment.
There is no need to define expected_target first. Just put it directly inside the call to assertEqual.
…me of downloaded content
|
@whimboo |
| log_level='ERROR') | ||
| self.assertEqual(scraper.target, destination) | ||
|
|
||
| # destination directory does not exist |
There was a problem hiding this comment.
I would also like to see the case when a filename has been specified and the folder does not exist yet. Maybe even multiple levels deep, so we can ensure that we correctly create a folder tree.
|
Except the one missing test this PR looks great! I also tested it and weren't able to find a problem. @davehunt would you mind to have a look at, so we can make sure this change satisfies your needs? |
|
@whimboo |
|
This looks great now! Thanks for your tireless work @ujjwalwahi! Given that Travis is also happy this time let me get it merged in a bit. |
|
When squashing the branch I got some rebase problems, so I had to fix those before pushing out. Finally the PR got merged as 942d972. |
@whimboo
Replaced directory(-d, --directory) with destination(-d, --dest), now user can specify filename of the download content. Also updated test cases to test added functionality. This will fix issue #92.