Skip to content

Conversation

@IsaacCloos
Copy link
Contributor

@IsaacCloos IsaacCloos commented Apr 5, 2023

Pull Request

This work addresses a slew of inconsistencies I've noticed in this projects documentation. These were found while trying to establish a basis to abide to for new doc comments coming soon to resolve #372

Related issue

Partially addresses #372

What does this PR do?

  • hide client instantiation from example view. It was 50/50 throughout the project so I push the 'hide' strategy. I personally believe it cleans up the method examples, but if you'd like to have it visible everywhere instead I can totally make that change 😄. I just think it would be beneficial to have some consistency here for users. (making the docs more predictable and all)
  • removed a few ```rust annotations (not the code!!). Per the docs stating rust is the default language.
  • made assertions visible where possible in the docs. This was 50/50 throughout and I believe that rounding out an example with a print or assert command is a better user experience.
  • implemented clippy recommendations in every existing doc comment in the project. Not ALL suggestions, but most. Lots of tab corrections, which I think make the comments much nicer to read.
  • fixed spacing inconsistencies in assert(s). E.g. .len()>0 changed to .len() > 0
  • bolded Default declarations (a trend I've noticed in other libraries).
  • did my best to split up paragraphs of text where I believe the intention was for the text to be separated but markdown puts new lines together if they are just one line return apart. Some were using the backtick method, but I changed them all to be double returns.
  • added a handful of missing references: [Key], [Index], ext
  • hid additional references making examples more concise
  • hid certain missed comments and closures that resulted in jarring or inaccurate examples

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

Criticism Welcome

I thought being a little opinionated about this first pass at a general "clean up" could prove fruitful if it sparked discussions about what YOU want to see in this project. I'd be more than happy to make any necessary changes to get this the way you'd like. If you have any questions about decisions I made, please let me know! I'm new to Rust and always trying to understand the dos and don'ts better 😄

Thanks for the feedback!

p.s. I am aware that this PR is everything BUT what the linked ticket is requesting 😅. I call it 'Preliminary work' 😆

Applying `cargo clippy` suggestions, spell checking, and cleaning up where project norms are not upheld.

*Please see PR for methodology and full list of explanations*
@bidoubiwa
Copy link
Contributor

Hey @IsaacCloos, sorry for the delay I was on holiday! I'm reviewing this now :)

@bidoubiwa
Copy link
Contributor

Seems there is an issue with the rust format. Could you check that ?

@IsaacCloos
Copy link
Contributor Author

🤦🏻‍♂️

cargo check, clippy, test, and fmt 😄

Also, do you know of a way I can opt out of the "auto issue close" workflow? That wasn't my intention until the issue is completely resolved.

@bidoubiwa
Copy link
Contributor

Since you didn't use the keywords fixes or closes it shouldn't auto-close the issue I think

Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

I went over all the changes, and they all seem good 🔥 Thanks so much for this amazing work

@bidoubiwa
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented Apr 24, 2023

Build succeeded:

@bors bors bot merged commit 30dc730 into meilisearch:main Apr 24, 2023
@bidoubiwa bidoubiwa added the skip-changelog The PR will not appear in the release changelogs label Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changelog The PR will not appear in the release changelogs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add documentation on builder methods.

2 participants