Skip to content

Advanced search filters - #1

Open
rabbitflyer wants to merge 3 commits into
talleyhoe:mainfrom
rabbitflyer:feature/filters
Open

Advanced search filters#1
rabbitflyer wants to merge 3 commits into
talleyhoe:mainfrom
rabbitflyer:feature/filters

Conversation

@rabbitflyer

Copy link
Copy Markdown

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

@talleyhoe

Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread src/scraper.py Outdated
if not os.path.exists(directory):
os.makedirs(directory)

global search_url

Copy link
Copy Markdown
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

Comment thread src/scraper.py Outdated
Comment thread src/scraper.py Outdated
else:
return ''

def process_safesearch(val: str):

Copy link
Copy Markdown
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"?

Comment thread src/scraper.py Outdated
if (subfeatures[1] != []):
features += ["safe=" + delim2.join(subfeatures[1])]

print(delim1.join(features))

Copy link
Copy Markdown
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

Comment thread src/main.py Outdated
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
Copy Markdown
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
Copy Markdown
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