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

Combine suggester restuls. #2681

Merged

Conversation

Hyperkid123
Copy link
Contributor

@Hyperkid123 Hyperkid123 commented Nov 6, 2023

Combine results of the default and the improved suggester results.

@codecov-commenter
Copy link

codecov-commenter commented Nov 6, 2023

Codecov Report

Merging #2681 (52c5070) into master (84c22e5) will increase coverage by 4.89%.
Report is 1 commits behind head on master.
The diff coverage is n/a.

❗ Current head 52c5070 differs from pull request most recent head 9c26a73. Consider uploading reports for the commit 9c26a73 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2681      +/-   ##
==========================================
+ Coverage   52.32%   57.21%   +4.89%     
==========================================
  Files          95       95              
  Lines        2515     2861     +346     
  Branches      456      588     +132     
==========================================
+ Hits         1316     1637     +321     
- Misses        827     1223     +396     
+ Partials      372        1     -371     
Files Coverage Δ
src/components/Search/SearchTypes.ts 100.00% <ø> (ø)

... and 59 files with indirect coverage changes

Copy link
Contributor

@karelhala karelhala left a comment

Choose a reason for hiding this comment

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

Looking good, just 2 styling issues.

function parseSuggestions(suggestions: SearchAutoSuggestionResultItem[] = []) {
return suggestions.map((suggestion) => {
const [allTitle, bundleTitle, abstract] = suggestion.term.split(AUTOSUGGEST_TERM_DELIMITER);
const url = new URL(suggestion.payload);
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to directly destruct the pathname here? const { pathname} = ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a class, not an object. Destructing classes might be dangerous.

const [allTitle, bundleTitle, abstract] = suggestion.term.split(AUTOSUGGEST_TERM_DELIMITER);
const url = new URL(suggestion.payload);
const pathname = url.pathname;
const item = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there specific reason to assign this to a const instead of directly constructing it when returning value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

None apart from better readability.

@Hyperkid123 Hyperkid123 force-pushed the use-improved-suggester branch 2 times, most recently from 9c26a73 to a1f645d Compare November 6, 2023 11:58
@Hyperkid123 Hyperkid123 force-pushed the use-improved-suggester branch from a1f645d to f10f1f7 Compare November 6, 2023 11:58
@Hyperkid123 Hyperkid123 merged commit d2df52b into RedHatInsights:master Nov 6, 2023
6 checks passed
@Hyperkid123 Hyperkid123 deleted the use-improved-suggester branch November 6, 2023 12:34
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.

3 participants