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

Improving RPC doc comments - addresses #339 #358

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

elielnfinic
Copy link
Contributor

@elielnfinic elielnfinic commented Feb 2, 2025

Improving RPC doc comments - addresses #339

@elielnfinic elielnfinic changed the title El/doc/rpc doc Improving doc comments - Fixes #339 Feb 2, 2025
@elielnfinic elielnfinic changed the title Improving doc comments - Fixes #339 Improving RPC doc comments - Fixes #339 Feb 2, 2025
docs/src/SUMMARY.md Outdated Show resolved Hide resolved
@elielnfinic elielnfinic marked this pull request as ready for review February 3, 2025 12:45
@dan-da
Copy link
Collaborator

dan-da commented Feb 4, 2025

thx for the PR.

small nit: I would say it addresses #339, not fixes it. That's because we divided it into two parts. Please edit the description of this PR, otherwise I believe github may automatically close #339 when this gets merged.

Copy link
Collaborator

@dan-da dan-da left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @elielnfinic, it's a good start.

Please make the changes I suggested in the comments and ensure that cargo test --doc executes cleanly without warnings/errors.

Also, please squash all the commits into one.
https://www.baeldung.com/ops/git-squash-commits

with that, I think we will be ready to merge it.

let me know if any q's or concerns.

@@ -204,150 +204,944 @@ pub trait RPC {
/// The CookieHint provides a location for the cookie file used by this
/// neptune-core instance as well as the [Network].
///
/// ```no_run
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ler's keep these examples focused on just the RPC call that is being documented. example:

    /// ```no_run
    /// # use neptune_cash::rpc_server::RPCClient;
    /// # use neptune_cash::rpc_auth;
    /// # use tarpc::tokio_serde::formats::Json;
    /// # use tarpc::serde_transport::tcp;
    /// # use tarpc::client;
    /// # use tarpc::context;
    ///
    /// # tokio_test::block_on(async {
    ///
    /// # let transport = tcp::connect("127.0.0.1:9799", Json::default).await.unwrap();
    /// # let client = RPCClient::new(client::Config::default(), transport).spawn();
    ///
    /// // query neptune-core server how to find the cookie file
    /// let cookie_hint = client.cookie_hint(context::current()).await.unwrap().unwrap();
    ///
    /// # })
    /// ```

The module level doc-comment already provides a full example showing use statements and how to instantiate rpc client and obtain auth token.

Copy link
Collaborator

Choose a reason for hiding this comment

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

also, can we use ?? instead of .unwrap().unwrap()?

/// let client = RPCClient::new(client::Config::default(), transport).spawn();
///
/// // load the cookie file from disk and assign it to a token
/// let token : rpc_auth::Token = rpc_auth::Cookie::try_load(&cookie_hint.data_directory).await.unwrap().into();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see cookie_hint defined. Does this actually compile? I think you are missing a call like:

    let cookie_hint = client.cookie_hint(context::current()).await??;

anyway, let's comment out all the setup stuff, so only the RPC call displays in the generated docs.

also, please replace the unwrap() with ? if possible.

@elielnfinic elielnfinic changed the title Improving RPC doc comments - Fixes #339 Improving RPC doc comments - addresses #339 Feb 4, 2025
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.

3 participants