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

Fixed broken search of series names #132

Merged
merged 6 commits into from
May 4, 2023
Merged

Conversation

bigdrex16
Copy link
Contributor

The current series search mechanism is broken. This fix is using the current method the web interface is working with: loading a JSON file beforehand, then searching it accordingly.
In the future, it would be better to preload it only once, when we start the console app, but for now it downloads the JSON file every time when performing a search.
Nevertheless: the search is currently working with the suggested fix.

@yairp03 yairp03 added type/bug Something isn't working area/horadot-api Related to the HoradotAPI project labels May 4, 2023
@bigdrex16
Copy link
Contributor Author

@yairp03 I'm not sure if it's something on my end or not. The failed test is due to this line:
Assert.AreEqual(75, (await driver.SearchSeries("ana")).Count());

It returns 77 shows instead of 75. There are 2 options: My comparison is different from what you've done before, or 2 more shows with 'ana' were added to Sdarot (which makes sense to me).

My comparison logic is pretty simple: it retrieves any show that contains the searched string, and ignore the case. It also checks for this string both in the English and Hebrew naming properties.

I will update the testing to look for 77 shows (instead of 75), by I'm not sure if the testing in general supposed to test for an exact number if it keeps changing (and it probably will). Let me know what you thing.

Let me know if there's anything else we want in order to merge this update, as the latest release is currently unusable without this fix.

@yairp03
Copy link
Owner

yairp03 commented May 4, 2023

Hey, your fix is great, I need to change the tests to be more general.
Let me review the commits and then we'll proceed

Copy link
Owner

@yairp03 yairp03 left a comment

Choose a reason for hiding this comment

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

Great fix, just a few changes and we are good to go

SdarotAPI/Models/SeriesInformation.cs Outdated Show resolved Hide resolved
SdarotAPI/Models/SeriesInformation.cs Show resolved Hide resolved
SdarotAPI/Models/SeriesInformation.cs Outdated Show resolved Hide resolved
SdarotAPI/Resources/Constants.cs Outdated Show resolved Hide resolved
SdarotAPI/SdarotDriver.cs Outdated Show resolved Hide resolved
SdarotAPI/SdarotDriver.cs Outdated Show resolved Hide resolved
Copy link
Owner

@yairp03 yairp03 left a comment

Choose a reason for hiding this comment

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

Some more small changes

SdarotAPI/Models/SeriesInformation.cs Outdated Show resolved Hide resolved
SdarotAPI/SdarotDriver.cs Outdated Show resolved Hide resolved
SdarotAPI/SdarotDriver.cs Show resolved Hide resolved
SdarotAPI/SdarotDriver.cs Show resolved Hide resolved
SdarotAPI/SdarotDriver.cs Outdated Show resolved Hide resolved
SdarotAPI/SdarotDriver.cs Outdated Show resolved Hide resolved
SdarotAPI/SdarotDriver.cs Show resolved Hide resolved
@yairp03 yairp03 linked an issue May 4, 2023 that may be closed by this pull request
@yairp03 yairp03 added this to the 1.4.0 milestone May 4, 2023
SdarotAPI/Models/SeriesInformation.cs Outdated Show resolved Hide resolved
SdarotAPI/Models/SeriesInformation.cs Outdated Show resolved Hide resolved
SdarotAPI/SdarotDriver.cs Show resolved Hide resolved
SdarotAPI/SdarotDriver.cs Outdated Show resolved Hide resolved
Copy link
Owner

@yairp03 yairp03 left a comment

Choose a reason for hiding this comment

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

Good job

@yairp03 yairp03 merged commit 9d4088d into yairp03:master May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/horadot-api Related to the HoradotAPI project type/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix search isn't working
2 participants