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

doced example acl_dryrun command #75

Conversation

ArtemIsmagilov
Copy link
Contributor

The lsp server(rust-analyzer) will perfectly show an example of this command.

@ArtemIsmagilov
Copy link
Contributor Author

ArtemIsmagilov commented Nov 6, 2024

@mcatanzariti, I would like to clarify with you. What comments do you think would best demonstrate the work of the methods?

My suggestions.

  1. Use ignore to specify in 1-2 lines what the request looks like.

  2. Use # symbols to hide all dependencies for running doc tests. Each time you need to copy the general template.

The first option is short, simple, allows you to easily change this documentation in case of adding a new runtime or possibly a synchronous interface (which is very useful when there is no need for asynchrony or when a thread pool is also suitable).

The second option requires a lot of hidden code, but the tests are passed. But we already have tests covering the code base in tests/ and I just want to quickly look at an example.

Which would be preferable for you? Maybe you have your own option?

I would be happy to add 1-2 lines with the ignore flag

    /// # Example
    /// ```ignore
    /// let result: String = client
    ///    .acl_dryrun("VIRGINIA", "GET", AclDryRunOptions::default().arg("foo"))
    ///    .await?;
    /// assert_eq!(
    ///    "User VIRGINIA has no permissions to run the 'get' command",
    ///    result
    /// );
    ///```
    /// 

ignore attr - https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html
# exclude symbol - https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html#hiding-portions-of-the-example

@ArtemIsmagilov
Copy link
Contributor Author

ArtemIsmagilov commented Nov 6, 2024

On the other hand, rust adheres to the rule of strict compliance of the example with its operability. As a Python developer, this approach is new to me. So, what's best for this project?

@mcatanzariti
Copy link
Member

@ArtemIsmagilov,

Interesting discussion, it was also new to me when I started writing Rust code, 2 years ago.

On one hand, I understand the point that having more boilerplate lines than the 2 actual lines that demonstrate the API for the sake of having a test that not only compiles but also does not fail can be really frustrating and it's probably why I don't have so many examples in the documentation.

On the other hand, Rust is the first language that offers me the guaranty that examples exposed in the documentation are not obsolete whenever the API or its implementation evolves.

Maybe it's ok to have ignorable examples to illustrate each Redis command as you are proposing it and at the same time having testable examples for the modules' introduction, as I have already implemented them.

Cheers,

Michaël

@ArtemIsmagilov
Copy link
Contributor Author

Screenshot from 2024-11-07 02-46-48

@ArtemIsmagilov
Copy link
Contributor Author

great! I used hidden symbols that test the code and at the same time leave only the necessary information for the user. Thanks for your comment

@mcatanzariti
Copy link
Member

Sorry, documentation test don't pass

@ArtemIsmagilov
Copy link
Contributor Author

@mcatanzariti, Hi! Thanks for the review. It looks like I made a mistake in the tests, in the comments.

Let me fix this

@mcatanzariti mcatanzariti merged commit 9cce41d into dahomey-technologies:main Nov 10, 2024
3 checks passed
@ArtemIsmagilov ArtemIsmagilov deleted the example-in-docstring-acl_dryrun branch November 10, 2024 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants