jtara1 / imgur_downloader

Python script/class to download an entire Imgur album in one go into a folder of your choice.
MIT License
40 stars 7 forks source link

Feature/parse only #4

Closed rachmadaniHaryono closed 7 years ago

rachmadaniHaryono commented 7 years ago

WIP

not yet commited

i test it using this imgur album http://imgur.com/a/stfnm (nsfw) and got different result between new method with json and old method with regex. maybe there is a way to double check the album using both method.

opinion @jtara1?

e: update after succesful test

jtara1 commented 7 years ago

What's the reason for using click over the built in lib, argparse https://docs.python.org/3/library/argparse.html

rachmadaniHaryono commented 7 years ago

simple and clearer arg command(see below) and more utils (which may or may not needed) and better subcommand.

for usual argparse in most of the program i read is located on a function. example:

def parse_args(argv):
    parser = argparse.ArgumentParser(description='Process some integers.')
    parser.add_argument('integers', metavar='N', type=int, nargs='+',
                    help='an integer for the accumulator')
    parser.add_argument('--sum', dest='accumulate', action='store_const',
                    const=sum, default=max,
                    help='sum the integers (default: find the max)')
    # ... another argument here
    return parser.parse_args(argv)

# after this function is called the actual function is called and the args 
# actual main 
def main():
    args = parse_args(sys.argv[1:])
    command(args)    

for me it is kinda redundant, and click can possibly short that all out.

@click.command()
@click.option(...)
@click.argument(...)
def main(...):
    # function here
jtara1 commented 7 years ago

I added the list of all imgur urls I expect this lib to support to the test file https://github.com/jtara1/imgur_downloader/blob/master/tests/test_image_ids_and_extensions.py

Feel free to add some test functions there.

If your json_search returns a nice dict with all that info like that for all of those urls then we could refactor a bunch of the code to just use that.

rachmadaniHaryono commented 7 years ago

can i refactor that test? it look like it can use pytest parametrize

jtara1 commented 7 years ago

yea

jtara1 commented 7 years ago

Ready to merge with master?

rachmadaniHaryono commented 7 years ago

Not yet. Maybe I can't complete it today (my laptop got error and I write response it from my tablet)

I will let you know if it is complete and cross WIP sign on first post

E:wrong button pushed

rachmadaniHaryono commented 7 years ago

done with the test already, i will update the first post based on change from the last comment.

travis check result

https://travis-ci.org/rachmadaniHaryono/imgur_downloader/builds/245297071

for the readme section, i will update it tomorow

rachmadaniHaryono commented 7 years ago

i think it is done for now. i may replace or give the user choice to which parser they choose and change the default to json parser on next pr. right now this is enough for print-only option.