Skip to content

Adaptation to Python 3, new features, bug fixes#58

Open
jtara1 wants to merge 46 commits intoHoverHell:masterfrom
jtara1:merge-ready
Open

Adaptation to Python 3, new features, bug fixes#58
jtara1 wants to merge 46 commits intoHoverHell:masterfrom
jtara1:merge-ready

Conversation

@jtara1
Copy link
Contributor

@jtara1 jtara1 commented Sep 14, 2016

If possible, please merge this pull request into a new branch.

My fork has been adapted to Python 3. Here's a summary of my changes in my merge-ready branch:

  • Python 3 only
  • --num cli arg counts by reddit submission rather than individual image from imgur
  • file ._history.txt stores last reddit id downloaded tied to local directory, subreddit name, & subreddit sort_type which is used to resume download position on the subreddit when the 3 aforementioned are matched.
  • arguments changed
    • <subreddit> will autodetect whether a text file or subreddit name was passed
    • --restart begin downloading from the beginning of the subreddit
    • --subreddit-list srl-filename download from each of the subreddits found in a text file

In addition to these changes there are fixes to the original code.

See my readme for more details
https://github.com/jtara1/RedditImageGrab/tree/merge-ready#jtara1-fork

ohyou and others added 30 commits June 19, 2016 12:15
@jtara1 jtara1 changed the title Python 3 - Merge ready Adaptation to Python 3, new features, bug fixes Sep 14, 2016
Copy link
Contributor

@rachmadaniHaryono rachmadaniHaryono left a comment

Choose a reason for hiding this comment

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

need a lot of change until it can be merged. and also testing is required.

any JPEG or PNG formatted image that it found listed in the specified
subreddit and download them to a folder.

## jtara1 Fork
Copy link
Contributor

Choose a reason for hiding this comment

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

only for fork

README.md Outdated

* \-\-num cli argument now counts by reddit submission rather than individual image

* added my fork (jtara1/imgur-downloader) to handle all imgur downloads (making the above feature possible)
Copy link
Contributor

Choose a reason for hiding this comment

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

fork only info. maybe change it into

added new library to handle ...

README.md Outdated
* added my fork (jtara1/imgur-downloader) to handle all imgur downloads (making the above feature possible)


* file .\_history.txt contains reddit id of last downloaded and is identified by subreddit & ARGS.sort\_type, e.g.:
Copy link
Contributor

Choose a reason for hiding this comment

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

file ._history.txt contains reddit id of last downloaded and is identified by subreddit & ARGS.sort_type, e.g.

README.md Outdated

> {'wallpapers': {'topmonth': {'last\-id': '4x4so2'}}}

* positional arguments, \<subreddit\> and \<dest\_file\>, changed to optional cli arguments
Copy link
Contributor

Choose a reason for hiding this comment

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

positional arguments, <subreddit> and <dest_file>, changed to optional cli arguments

README.md Outdated

* positional arguments changed

* --dir is required
Copy link
Contributor

Choose a reason for hiding this comment

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

if dir is required, don't use -- prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think its ok to revert back to using two positional arguments but have the first autodetect whether it's a subreddit name, multireddit name, or subreddit-list file?

Copy link
Contributor

Choose a reason for hiding this comment

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

it is possible. use os.path.isfile to check if input is filename and check if it have .txt extension. something similar like this

if os.path.isfile(filename) and os.path.splitext(filename)[1] == '.txt':

f.write(str(write_data))
return True
else:
print ('Error in historyLog func: invalid mode')
Copy link
Contributor

Choose a reason for hiding this comment

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

use logging.debug so user don't see any print from this debug func.

write_data: any - only relevant if mode == 'write', output is converted to string and written to file_log
'''
path = os.path.join(os.getcwd(), wdir) if not os.path.isdir(wdir) else wdir
if mode == 'read':
Copy link
Contributor

@rachmadaniHaryono rachmadaniHaryono Sep 15, 2016

Choose a reason for hiding this comment

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

mode_dict = {
    'read': 'r',
    'write': 'w',
   'append': 'a'
}
if mode in mode_dict:
    with open(os.path.join(path, file_log), mode_dict[mode]) as f:
        if mode == 'read':
            # http://nedbatchelder.com/blog/201206/eval_really_is_dangerous.html
            return eval(f.read())  # this is dangerous
        else:
            f.write(str(write_data))
            return True
else:
    logging.debug(dbg_message)
    # return False  # ?

start_time = time.clock()
if ARGS.subreddit_list:
ARGS.subreddit_list = ARGS.subreddit_list[0]
SUBREDDIT_FILE = os.path.join(os.getcwd(), ARGS.subreddit_list)
Copy link
Contributor

Choose a reason for hiding this comment

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

variable not written in capital letters. old arguments are lot like this and yet not fixed. we don't want to continue that.

@@ -0,0 +1,3 @@
pc-wallpapers:
Copy link
Contributor

Choose a reason for hiding this comment

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

need more example. function doc have more.

import os, sys
from os import getcwd

sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
Copy link
Contributor

@rachmadaniHaryono rachmadaniHaryono Sep 15, 2016

Choose a reason for hiding this comment

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

does it have to append it to system path? if you use nose or pytest i think it can find it.

@rachmadaniHaryono
Copy link
Contributor

rachmadaniHaryono commented Sep 15, 2016

subreddit file list is a good feature, but it still need a lot improvement.

here is an example how text file can be used as input list.

https://github.com/Bakkingamu/tumblr_image_download_script

it support commenting and category(in our case we can use sort type etc).

try also documenting the function and method. use google doc style or sphinx style. if it goes right, we can build sphinx docs from the project.

@jtara1
Copy link
Contributor Author

jtara1 commented Sep 15, 2016

Thanks for all the feedback; these mostly look like small changes. I haven't collaborated on programming projects like much until recently.

@jtara1
Copy link
Contributor Author

jtara1 commented Sep 17, 2016

Here's most of the changes @rachmadaniHaryono recommended. In addition there's a critical fix for imgur-downloader, and a new feature, --restart command line interface argument.

Not sure if my readme.md is exactly how you'd like.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants