-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Split retrievers docs #131385
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
Split retrievers docs #131385
Conversation
Pinging @elastic/es-docs (Team:Docs) |
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.
Looks good to me, thanks for taking care of this @leemthompo ! I've left some non-blocking feedback.
docs/reference/elasticsearch/rest-apis/retrievers/knn-retriever.md
Outdated
Show resolved
Hide resolved
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.
For the most part, these retrievers are being added in the order in which they were developed, which isn't very helpful.
I'd like to suggest thinking about how we will organize this list long term as we continue to add new retrievers (for example break out standard vs compound retriever use cases), but in the meantime, perhaps we can order the retrievers in this list alphabetically? This would make it easier for users to scan for the particular retriever they're looking for?
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.
makes sense, I ordered them alphabetically in the TOC also so I should be consistent :) 👍
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.
long term we'll document these in the spec hopefully but the current api docs frontend doesn't really showcase retrievers very well, but as you say that's a longer term consideration
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.
++ for grouping retrievers by type in the future
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.
I mean I could maybe do this in this PR if we wanna get that ball rolling 🤷♂️
Lemme know what you think
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.
I'd say merge as is for now and do any other reorganization as a followup - this is already a great change. Thanks!
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! I'm not sure how redirects work in the docs, so I didn't review that part. Otherwise, I think @kderusso's comments cover it :)
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.
++ for grouping retrievers by type in the future
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.
I'd say merge as is for now and do any other reorganization as a followup - this is already a great change. Thanks!
💚 Backport successful
|
Closes https://github.com/elastic/developer-docs-team/issues/322