Skip to content

Advanced search filters #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rabbitflyer
Copy link

Added a bunch of arguments corresponding to the various advanced image search filters and the safesearch setting.

@talleyhoe
Copy link
Owner

Hey, I'm pretty busy this week so I probably won't have time to review/merge until the weekend. Great idea! At first glance, code looks good. Thanks for following the style :)

@talleyhoe
Copy link
Owner

There's 2 big issues that have to be fixed (I'll help)

1.) The filters should all be optional

2.) Each option needs to be tested. This includes all of the choices for categorical arguments (large, medium, tall, square, etc)

3.) Plz try to avoid rows longer than 80 columns

src/scraper.py Outdated
@@ -174,7 +259,8 @@ def scrape_images(search_key, image_cnt, directory, threads):
print("savedir: {}".format(directory))
if not os.path.exists(directory):
os.makedirs(directory)

global search_url
Copy link
Owner

Choose a reason for hiding this comment

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

The global should be called in setup_url so the logic is compartmentalized

src/scraper.py Outdated
return key + 'ol'
else:
return ''

def process_safesearch(val: str):
Copy link
Owner

Choose a reason for hiding this comment

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

Is the default safesearch behavior any different from "off"?

src/scraper.py Outdated
@@ -122,7 +147,7 @@ def setup_url(searchurl: str, imgsize: str, imgcolor: str, imgtype: str, safesea
features += ["tbs=" + delim2.join(subfeatures[0])]
if (subfeatures[1] != []):
features += ["safe=" + delim2.join(subfeatures[1])]

print(delim1.join(features))
Copy link
Owner

Choose a reason for hiding this comment

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

Please wrap debugging print statements in an 'if DEBUG' and provide them useful text to identify the output

src/main.py Outdated
@@ -5,7 +5,7 @@

def main():
args = get_arguments(sys.argv)
scrape_images(args.keyword[0], args.count, args.directory, args.threads, args.size, args.color, args.imgtype, args.safesearch)
scrape_images(args.keyword[0], args.count, args.directory, args.threads, args.size, args.aspectratio, args.color, args.imgtype, args.region, args.filetype, args.usage, args.safesearch)
Copy link
Owner

Choose a reason for hiding this comment

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

Filters should be passed as a single object so we can try to avoid these long definitions that look ugly. IMO if you can't call the function on one line, it needs to be refactored into a more compact structure

@talleyhoe talleyhoe closed this Oct 11, 2023
@talleyhoe talleyhoe reopened this Oct 11, 2023
@rabbitflyer rabbitflyer marked this pull request as ready for review December 5, 2023 20:06
@rabbitflyer
Copy link
Author

Sorry, haven't revised/added anything, but realized that this can't be merged until it's "marked as ready".

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.

2 participants