Skip to content

Add API docs for Amount type#710

Merged
thunderbiscuit merged 1 commit intobitcoindevkit:masterfrom
thunderbiscuit:docs/amount-api-docs
Mar 31, 2025
Merged

Add API docs for Amount type#710
thunderbiscuit merged 1 commit intobitcoindevkit:masterfrom
thunderbiscuit:docs/amount-api-docs

Conversation

@thunderbiscuit
Copy link
Copy Markdown
Member

@thunderbiscuit thunderbiscuit commented Mar 25, 2025

Sorry I was too quick to merge #708! My bad.

This fixes that.

See https://docs.rs/bitcoin/latest/bitcoin/struct.Amount.html for details.

@thunderbiscuit thunderbiscuit force-pushed the docs/amount-api-docs branch 3 times, most recently from 3937e69 to c293ef4 Compare March 25, 2025 16:00
Itorouk

This comment was marked as duplicate.

Copy link
Copy Markdown
Collaborator

@ItoroD ItoroD left a comment

Choose a reason for hiding this comment

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

I have looked at the generated doc. Looks good. Only weird thing is the constructor doc. This was mentioned for TxBuilder here promarcos as well hence I believe its a known thing. Besides this I could go ahead and approve.

@thunderbiscuit
Copy link
Copy Markdown
Member Author

Yeah I agree we should look at exactly where to put the docstrings. Let me confirm where is best and I'll push the changes if there are any.

@thunderbiscuit
Copy link
Copy Markdown
Member Author

I have played around with docstring placement.

I think the best place to have the general type definition is on the struct line directly, with smaller documentation on the constructors and methods. Note that docstrings added to the impl block will simply be ignored.

Example

/// This is the definition used on the struct.
#[derive(Clone, Debug, uniffi::Object)]
pub struct Script(pub BdkScriptBuf);

/// This is the definition used on the impl block.
#[uniffi::export]
impl Script {
    /// This is the docstring for the constructor.
    #[uniffi::constructor]
    pub fn new(raw_output_script: Vec<u8>) -> Self {
        let script: BdkScriptBuf = raw_output_script.into();
        Script(script)
    }

    /// This is a docstring for a method.
    pub fn to_bytes(&self) -> Vec<u8> {
        self.0.to_bytes()
    }
}

Resulting API docs:

image

@rustaceanrob
Copy link
Copy Markdown
Collaborator

I see. When using the actual JVM docs this is the best configuration. My autocomplete was showing the constructor docs, but that's more of an IDE quark. We should go with the style you have here

@rustaceanrob
Copy link
Copy Markdown
Collaborator

ACK 807fd9f

@ItoroD

This comment was marked as resolved.

@ItoroD
Copy link
Copy Markdown
Collaborator

ItoroD commented Mar 28, 2025

I have just seen from testing and docs that using the #[uniffi::constructor] does not necessarily mean that that method will become a constructor in your language bindng (kotlin).

All good here!

@thunderbiscuit
Copy link
Copy Markdown
Member Author

thunderbiscuit commented Mar 31, 2025

@rustaceanrob @ItoroD good points. I also wanted to make sure I got correct behaviour on my IDE and decided to go one step further in my digging. Here is what I found after testing a bunch of stuff.

Setup

I used the Address type because it's a full object (interface in uniffi), and has a struct block, and impl block, a default constructor, an alternate constructor, and standard methods.

/// These are the type-level API docs declared on the struct block.
#[derive(Debug, PartialEq, Eq, uniffi::Object)]
pub struct Address(BdkAddress<NetworkChecked>);

/// These are the API docs declared on the impl block.
#[uniffi::export]
impl Address {
    /// These are the API docs declared on the default constructor.
    #[uniffi::constructor]
    pub fn new(address: String, network: Network) -> Result<Self, AddressParseError> {
        let parsed_address = address.parse::<bdk_wallet::bitcoin::Address<NetworkUnchecked>>()?;
        let network_checked_address = parsed_address.require_network(network)?;

        Ok(Address(network_checked_address))
    }

    /// These are the API docs declared on an alternative constructor.
    #[uniffi::constructor]
    pub fn from_script(script: Arc<Script>, network: Network) -> Result<Self, FromScriptError> {
        let address = BdkAddress::from_script(&script.0.clone(), network)?;

        Ok(Address(address))
    }

    /// These are API docs declared on a method.
    pub fn script_pubkey(&self) -> Arc<Script> {
        Arc::new(Script(self.0.script_pubkey()))
    }

Results

  • If you have docstrings on the type as well as the constructor, the overall type documentation appears in the tooltip when hovering over or requesting API docs on the type declaration if using explicit type declaration like so:
  • If you are instantiating a type using the default constructor however, the IDE will pull the docstring of the constructor if it exists, otherwise the overall type API docs will simply be displayed:
2
  • Calling a secondary constructor will display its API documentation if it exist, nothing otherwise:
4
  • Calling a method of course displays the method API docstring.
3
  • API docs written on the impl block never show up anywhere (they're not pulled into the glue code).

@thunderbiscuit
Copy link
Copy Markdown
Member Author

@ItoroD about the constructors:

Yes uniffi doesn't allow for constructor overloading, and so only one standard constructor can be created per type. Rust usually uses the Type::new() method to declare its main constructor, and uniffi pulls that method (new()) as the main constructor by default. If it exist, this is the method that will translate into something like Address(arg1, arg2). Other constructors will be named, and exist as static functions on the type (and are therefore put into the companion object), i.e. Address.fromScript() and Amount.fromSat().

The companion object has always been a little quirk of Kotlin that I don't know was ever that clear, and I have even heard Andrey Breslav say it was one of the things he regrets introducing (complexity that didn't add much). They're really just static methods on the type.

Pardon me going off on this if you're super familiar with Kotlin idioms I just wasn't sure.

@rustaceanrob
Copy link
Copy Markdown
Collaborator

If you are instantiating a type using the default constructor however, the IDE will pull the docstring of the constructor if it exists, otherwise the overall type API docs will simply be displayed

Yeah, in my experience this is the first thing that comes up when typing normally? Because the hover functionality to pull up the struct-level documentation exists, I think it is fine to leave it how we have it. In a way it makes sense, for most people they just want to know how to make an Electrum client for instance. The description of the client can be found for the curious by hovering.

@ItoroD
Copy link
Copy Markdown
Collaborator

ItoroD commented Mar 31, 2025

@ItoroD about the constructors:

Yes uniffi doesn't allow for constructor overloading, and so only one standard constructor can be created per type. Rust usually uses the Type::new() method to declare its main constructor, and uniffi pulls that method (new()) as the main constructor by default. If it exist, this is the method that will translate into something like Address(arg1, arg2). Other constructors will be named, and exist as static functions on the type (and are therefore put into the companion object), i.e. Address.fromScript() and Amount.fromSat().

The companion object has always been a little quirk of Kotlin that I don't know was ever that clear, and I have even heard Andrey Breslav say it was one of the things he regrets introducing (complexity that didn't add much). They're really just static methods on the type.

Pardon me going off on this if you're super familiar with Kotlin idioms I just wasn't sure.

That makes sense. 👍

@thunderbiscuit thunderbiscuit merged commit 807fd9f into bitcoindevkit:master Mar 31, 2025
26 checks passed
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.

4 participants