Edits for overall doc review #52
Conversation
github-actions
bot
added
the
documentation
This comment was marked as resolved.
This comment was marked as resolved.
…pymeilisearch into doc/overall_edit
…pymeilisearch into doc/overall_edit
…arch into doc/overall_edit
…arch into doc/overall_edit # Conflicts: # src/ansys/tools/meilisearch/cli.py
| UR without the ``https://``. The default is ``None``, in which | ||
| case the unique ID of the first URL specified for the ``urls`` | ||
| parameter is used. | ||
| stop_urls_default : str, optional |
There was a problem hiding this comment.
@Revathyvenugopal162 We need to add a decription for the stop_urls_default parameter. We also need to add descriptions for two attributes that are blank on the summary table for the ansys.tools.meilisearch. templates`` page: DEFAULT_TEMPLATE and SPHINX_PYDATA_TEMPLATE. Also, STOP_SPHINX_URLS has all of the following parameters that show on the same page but are never defined. I realize that all the ones beginning wiht the underscore won't display, but I don't know about the last one. I also know that Alex always believed that even hidden methods and parameters should be documented.
"_sources",
"_downloads",
"_static",
"_images",
".doctree",
| @@ -1,40 +1,40 @@ | |||
| """Provides the templates utils module.""" | |||
| """Templates utilities module.""" | |||
There was a problem hiding this comment.
@Revathyvenugopal162 In this PY file, the fetch_all_documents() method in the MeilisearchUtils class has a ``limits" parameter. I don't really understand the description: "Limit of document in single offset." I was hoping you can improve this description.
There was a problem hiding this comment.
Thanks for noting this, Modified the docstring in 2a00920
| index_uid : str, default: None | ||
| The destination index name | ||
| Name of the destination index. |
There was a problem hiding this comment.
@Revathyvenugopal162 It seems odd that the name of the parameter is index_uid but the desription says that you specify the name rather the unique ID of the destination index. Is there something incorrect here?
There was a problem hiding this comment.
index uid is the name came from meilisearch and we are following the name, for ease of use of the id in front end, it is a name, i changed all the occurrence of id to name, as it can cause confusion. Thank you pointing this .Modified in 2a00920
|
@Revathyvenugopal162 This PR is ready for your review, acceptance, and even merging. If you want to check out this branch to resolve comments in which I've posed questions, please do. I've also noted some overall concerns in the PR description. You can address these concerns now or create an issue to address those that you agree with after this library's public release. If you want me to review the doc again, let me know. Otherwise, I will assume that the overall doc review required for public release is complete. |
|
@PipKat Thank you for the review.
|
I used "indexes" rather than "indices" because "indexes" is what is used in the Meillisearch doc. (While both are now acceptable spellings, because we are enhancing the Meillisearch API, I thought we should use the same term that they do.)
I find the image for the search bar component (_static/search.html) in the main index a bit confusing. It seems to be interactive, but it really isn't. Perhaps it should be a static image or even a short, animated GIF? Or, maybe we only need to describe in text how you can click in the "docs-searchbar input" area to see examples of the types of searches you might perform and use the dropdown for the box to the right to select the repository to search. However, the search bar shown is actually an implementation.
In getting-started/installing-pymeilisearch.rst, I find the "double" tab sets for the developer installation a bit much because the commands for CMD and PowerShell on Windows are the same. I'd suggest simplifying this by concluding each step (Such as "Start by cloning the repository:") with a period rather than existing colon. Then, use On Windows" and On Linux/UNIX headings with introductory text. For example, the text for the first step's On Windows section would be something like this: "On WIndows, use either CMD or PowerShell to run this command: (followed by the code block). The text for the first step's On Linux/UNIX section would be something like this: On Linux or UNIX, run this command in your terminal: (followed by the code block). I personally feel all those levels of tabs are a bit overwhelming and unnecessary. However, because others might not feel this way, it is totally your call as to whether you leave it as is or redo it (or have me redo it) as suggested.
I see different default ports. In user-guide/cli-usage.rst, the Options section says the default is 8000 for localhost. In getting-started/meilisearch-images.rst, it says port 7700 (for the local Meilisearch instance started by Docker). I'm not sure if this is a mistake or intentional.
On the API pages, the large title at the top with the module name runs off the page. I find this annoying. I'm sure this problem exists everywhere we are using automation to build API content.
Lastly, I feel that the "User guide" should include more information about the two available templates. I really am not sure what they do or when I might pick one template over another (or if I could/should possibly create additional templates). I doubt that we need to add a lot of content, but I think adding more information on this topic would be helpful.