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

explorer default sort by score #3689

Merged
merged 8 commits into from
Dec 6, 2023
Merged

explorer default sort by score #3689

merged 8 commits into from
Dec 6, 2023

Conversation

enekofb
Copy link
Contributor

@enekofb enekofb commented Dec 4, 2023

Closes #3627

What changed?

  • Using score by default ordering criteria over name.
  • Do not index unstructured in the regular document so we have more realistic scoring by the documents.

Why was this change made?

  • To have more relevant results when no ordering criteria is introduced

How did you validate the change?

  • Explain how a reviewer can verify the change themselves

Tilt on the branch and search for flux-system you should see now flux-system objects ranking first

Screenshot 2023-12-04 at 12 28 57

  • Integration tests -- what is covered, what cannot be covered;
    or, explain why there are no new tests

  • Unit tests -- what is covered, what cannot be covered; are
    there tests that fail without the change?

Added unit test to ensure deterministic search by scoring behaviour

Release notes

Documentation Changes

Other follow ups

@enekofb enekofb added the enhancement New feature or request label Dec 4, 2023
@enekofb enekofb changed the title Wge 3627 explorer sort score explorer default sort by score Dec 4, 2023
@enekofb enekofb force-pushed the wge-3627-explorer-sort-score branch from 64f34b9 to 0adf3dd Compare December 4, 2023 11:36
@enekofb enekofb requested a review from a team December 4, 2023 11:36
@enekofb enekofb marked this pull request as ready for review December 4, 2023 11:36
@enekofb enekofb closed this Dec 4, 2023
@enekofb enekofb reopened this Dec 4, 2023
Copy link
Contributor

@jpellizzari jpellizzari left a comment

Choose a reason for hiding this comment

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

👍 I like the extra test coverage and I think the results will make more sense.

@enekofb enekofb force-pushed the wge-3627-explorer-sort-score branch from 0adf3dd to fae56bc Compare December 6, 2023 07:39
},
want: []string{"someName", "otherName"},
want: []string{"someName"},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reducing the number of results given that without any ordering, the default is score, and given * there is no indication of quality ... we have both results with score of 1 ... which we cannot depend on its order reliability for asserting ...

Screenshot 2023-12-06 at 16 37 55

given the test is that we could query by * ... having one or more than one might not matter much

@enekofb enekofb requested a review from jpellizzari December 6, 2023 15:48
Copy link
Contributor

@jpellizzari jpellizzari left a comment

Choose a reason for hiding this comment

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

Still LGTM. The new test logic makes sense.

@enekofb enekofb force-pushed the wge-3627-explorer-sort-score branch from ee3e091 to 6dcb162 Compare December 6, 2023 18:11
@enekofb enekofb merged commit c94cfc7 into main Dec 6, 2023
10 checks passed
@enekofb enekofb deleted the wge-3627-explorer-sort-score branch December 6, 2023 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spike: try not sorting by name by default in Explorer
2 participants