-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Retail search samples with tests (new) #1628
Retail search samples with tests (new) #1628
Conversation
vasilypascal
commented
Feb 14, 2022
- Added retails search samples
- Added tests
- Created Runner project, which executes the sample by it's name with the help of ExampleAttributeHelper class
Here is the summary of changes. You are about to add 6 region tags.
This comment is generated by snippet-bot.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good.
I have some minor requests.
And also, about the pagination sample, something needs to be done there so we can test all the code. Either split the sample in three different samples or create three different methods, one for each pagination "mode" you want to show. If you think this is going to take some time, I'm happy if you remove that sample and its test from this PR, and add it in a following PR.
var firstPage = searchResultPages.ToArray()[0]; | ||
var thirdPage = searchResultPages.ToArray()[2]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this twice means that we fetch everything from the backend twice, i.e. all pages. In this case it is better to do something like:
var firstPage = searchResultPages.ToArray()[0]; | |
var thirdPage = searchResultPages.ToArray()[2]; | |
var topPages = searchResultPages.Take(3).ToList(); | |
var firstPage = topPages[0]; | |
var thirdPage = topPages[2]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Will improve that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved.
} | ||
} | ||
|
||
// Paste call with next page token here: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot tests all of this code because it is commented.
Also we don't use commented code in documentation samples.
I think it's best if you have a sample for each of these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the SearchWithPagination Sample for now by the agreement with @tetiana-karasova
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved.
retail/interactive-tutorial/RetailSearch.Samples.Tests/SearchWithOrderingTest.cs
Outdated
Show resolved
Hide resolved
retail/interactive-tutorial/RetailSearch.Samples.Tests/SearchWithFilteringTest.cs
Show resolved
Hide resolved
retail/interactive-tutorial/RetailSearch.Samples/SearchWithFacetSpecSample.cs
Show resolved
Hide resolved
@amanda-tarafa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks!
retail/interactive-tutorial/RetailSearch.Samples.Tests/SearchWithFilteringTest.cs
Show resolved
Hide resolved
Failures in CI are unrelated and being tracked in #1623. |