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

feat: filter models #261

Merged
merged 6 commits into from
Sep 24, 2020
Merged

feat: filter models #261

merged 6 commits into from
Sep 24, 2020

Conversation

admon84
Copy link
Contributor

@admon84 admon84 commented Sep 24, 2020

Description

Testing

Test 1 - no filtering on models

SHOW_ONLY_BRANDS="nvidia,evga,asus,msi"
SHOW_ONLY_MODELS=""
SHOW_ONLY_SERIES="3080"
STORES="nvidia,bestbuy,evga,asus,amazon,newegg"

Result:
Screenshot from 2020-09-23 20-07-12


Test 2 - filter on models

SHOW_ONLY_BRANDS="nvidia,evga,asus,msi"
SHOW_ONLY_MODELS="founders edition,rog strix"
SHOW_ONLY_SERIES="3080"
STORES="nvidia,bestbuy,evga,asus,amazon,newegg"

Results:
Screenshot from 2020-09-23 20-11-52

New dependencies

N/A

@admon84 admon84 marked this pull request as ready for review September 24, 2020 02:17
@admon84 admon84 requested a review from jef as a code owner September 24, 2020 02:17
@vp-regular
Copy link
Contributor

Suggestion:

I feel that this feature will be very error prone by the end user. Maybe we should use a string similarity algorithm like levenshtein distance (I have a Java implementation hat I can probably dig up...) or another lib to prevent something breaking because of something like the user entering rogstrix instead of rog strix?

@admon84
Copy link
Contributor Author

admon84 commented Sep 24, 2020

Suggestion:

I feel that this feature will be very error prone by the end user. Maybe we should use a string similarity algorithm like levenshtein distance (I have a Java implementation hat I can probably dig up...) or another lib to prevent something breaking because of something like the user entering rogstrix instead of rog strix?

I see what you're saying, but I wouldn't want to over-complicate it either. With string comparisons it's common to compare lower/upper case user input to a matching lower/upper case value to eliminate case mismatching issues.

Maybe in this case (for filtering models) we should remove all whitepsace when comparing the model name in the store config to what the user typed into their .env config

@vp-regular
Copy link
Contributor

Sounds like a plan; just giving my 2c.

@admon84
Copy link
Contributor Author

admon84 commented Sep 24, 2020

Latest commit adds whitespace sanitization, so at least it will be a little forgiving with user input having spacing not matching the store config: "gaming x trio" vs "gamingx trio" vs "gamingxtrio"
Screenshot from 2020-09-23 20-34-00

Copy link
Owner

@jef jef left a comment

Choose a reason for hiding this comment

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

Looks good! I like it. Nicely done with the regex.

One comment I will make is that doing regex over and over again like this isn't the best thing to do.

One thing we should do next is do the filtering all before the program starts to parse through the pages, that way we've done all the setup we need to do. That's outside of this, but just a comment.

Thanks!

@@ -0,0 +1,62 @@
import {Config} from '../config';
Copy link
Owner

Choose a reason for hiding this comment

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

This was on my todo list to refactor this out. Love it!

@jef jef merged commit e1b34a9 into jef:main Sep 24, 2020
@admon84 admon84 deleted the filter-model branch September 24, 2020 03:00
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.

Filter card brand and model
3 participants