Skip to content
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

Does photoquery.py have duplicate code (lines 578 vs 583)? #1731

Open
mravery opened this issue Nov 4, 2024 · 4 comments
Open

Does photoquery.py have duplicate code (lines 578 vs 583)? #1731

mravery opened this issue Nov 4, 2024 · 4 comments
Labels
bug Something isn't working refactoring Code that should be refactored

Comments

@mravery
Copy link

mravery commented Nov 4, 2024

Describe the bug
Starting in line 578 and ending 587, it looks like these two if blocks are exactly the same (i.e. you can delete one of them without affecting the logic).

To Reproduce
View the code at the above lines.

Screenshots
Screen Shot 2024-11-04 at 8 21 12 PM

Additional context
I was just browsing the code and came upon this. I am not well-versed in python, nor am I well-versed in the code-base. There could be a legitimate reason for this.

@RhetTbull
Copy link
Owner

You're right! Thanks for catching this. Not a bug technically but the code is unnecessary and doesn't actually do anything.

@RhetTbull
Copy link
Owner

@all-contributors please add @mravery for bug

@RhetTbull RhetTbull added bug Something isn't working refactoring Code that should be refactored labels Nov 4, 2024
Copy link
Contributor

@RhetTbull

I've put up a pull request to add @mravery! 🎉

@RhetTbull
Copy link
Owner

Looking at this code it should really be refactored to be simplified. If I change all the options to match the name of the PhotoInfo property, e.g. option.isreference and option.not_isreference then much of the code could be greatly simplified. (Today, some of the options have a different name than the PhotoInfo property, e.g. option.is_reference). We could then do something like this (after accounting for the text search properties like description:

def filter_photos(options: Options, photos: list[PhotoInfo]) -> list[PhotoInfo]:
    filtered_photos = photos
    for field in options.fields():
        if field.startswith("not_"):
            prop = field[4:]
            filtered_photos = [photo for photo in filtered_photos if not getattr(photo, prop, None)]
        else:
            filtered_photos = [photo for photo in filtered_photos if getattr(photo, field, None)]
    
    return filtered_photos

This would also make adding new options easier. There are a few special cases that would need to be handled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working refactoring Code that should be refactored
Projects
None yet
Development

No branches or pull requests

2 participants