-
Notifications
You must be signed in to change notification settings - Fork 124
Refactor search ranking #3424
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
Refactor search ranking #3424
Conversation
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.
nits only. Nice!
lib/src/search.dart
Outdated
final String name; | ||
final String qualifiedName; | ||
|
||
// TODO(srawlins): Store the integer of in the package order instead of this |
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.
nit: the index of the package in package order
or similar
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.
Thanks! Done
lib/src/search.dart
Outdated
// "name":"dartdoc", | ||
// "qualifiedName":"dartdoc", | ||
// "href":"dartdoc/dartdoc-library.html", | ||
// "type":"library", |
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 might be a field that could be crushed into an integer too, if needed.
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.
Yeah, it would be nice. We actually display the "type" in the search result so we would have to store the "type" as an int (rather convert to a scope), but yes still savings.
lib/src/search.dart
Outdated
// ```dart | ||
// { | ||
// "name":"dartdoc", | ||
// "qualifiedName":"dartdoc", |
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.
nit: example as "libName.variableName" might be better?
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.
Done
); | ||
} | ||
|
||
int get _scope => |
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.
A comment explaining this would be nice.
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.
Good call, done! And added a TODO, as its not well-tested.
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.
Very cool!
} | ||
|
||
@reflectiveTest | ||
class SearchTest { |
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.
👍
A few items of note:
lib/
and add rudimentary tests. (work towards search UI redux #3419)--package-order
flag. (work towards Search results rankings are not always what I'd expect #3176)