Skip to content

Conversation

x0rw
Copy link
Contributor

@x0rw x0rw commented Jul 2, 2025

What problem are we solving?

Currently to initiate a consul type you have to do

    let payload = RegisterEntityPayload {
        ID: None,
        Node: node_id.to_string(),
        Address: "127.0.0.1".to_string(),
        Datacenter: None,
        TaggedAddresses: Default::default(),
        NodeMeta: Default::default(),
        Service: Some(RegisterEntityService {
            ID: None,
            Service: service_name.to_string(),
            Tags: vec![],
            TaggedAddresses: Default::default(),
            Meta: Default::default(),
            Port: Some(42424),
            Namespace: None,
        }),
        Checks: Vec::new(),
        SkipNodeUpdate: None,
    };

which is very repetitive, verbose and hurts the eye.

How are we solving the problem?

This PR introduces Bon that automatically generates idiomatic setters for all named fields in a struct.
And the client can do this:

        let test_service = RegisterEntityService::builder()
            .Service("service".to_owned())
            .port(10000)
            .build();
    
        let _ = RegisterEntityPayload::builder()
            .Node("node-ddd".to_owned())
            .Address("2.2.2.2".to_owned())
            .Service(test_service)
            .build();       

Public Api

correspondant public api(reduced):
cargo public-api diff main..HEAD -sss

Removed items from the public API
=================================
(none)

Changed items in the public API
===============================
(none)

Added items to the public API
=============================
+pub struct rs_consul::acl_types::ACLTokenPolicyLinkBuilder<S: rs_consul::acl_types::a_c_l_token_policy_link_builder::State>
+pub struct rs_consul::acl_types::CreateACLPolicyRequestBuilder<S: rs_consul::acl_types::create_a_c_l_policy_request_builder::State>
+pub struct rs_consul::acl_types::CreateACLTokenPayloadBuilder<S: rs_consul::acl_types::create_a_c_l_token_payload_builder::State>
+pub struct rs_consul::types::CreateOrUpdateKeyRequestBuilder<'a, S: rs_consul::types::create_or_update_key_request_builder::State>
+pub struct rs_consul::types::DeleteKeyRequestBuilder<'a, S: rs_consul::types::delete_key_request_builder::State>
+pub struct rs_consul::types::DeregisterEntityPayloadBuilder<S: rs_consul::types::deregister_entity_payload_builder::State>
+pub struct rs_consul::types::GetNodesRequestBuilder<'a, S: rs_consul::types::get_nodes_request_builder::State>
+pub struct rs_consul::types::GetServiceNodesRequestBuilder<'a, S: rs_consul::types::get_service_nodes_request_builder::State>
+pub struct rs_consul::types::LockRequestBuilder<'a, S: rs_consul::types::lock_request_builder::State>
+pub struct rs_consul::types::LockWatchRequestBuilder<'a, S: rs_consul::types::lock_watch_request_builder::State>
+pub struct rs_consul::types::ReadKeyRequestBuilder<'a, S: rs_consul::types::read_key_request_builder::State>
+pub struct rs_consul::types::RegisterEntityCheckBuilder<S: rs_consul::types::register_entity_check_builder::State>
+pub struct rs_consul::types::RegisterEntityPayloadBuilder<S: rs_consul::types::register_entity_payload_builder::State>
+pub struct rs_consul::types::RegisterEntityServiceBuilder<S: rs_consul::types::register_entity_service_builder::State>
+pub struct rs_consul::ACLTokenPolicyLinkBuilder<S: rs_consul::acl_types::a_c_l_token_policy_link_builder::State>
+pub struct rs_consul::CreateACLPolicyRequestBuilder<S: rs_consul::acl_types::create_a_c_l_policy_request_builder::State>
+pub struct rs_consul::CreateACLTokenPayloadBuilder<S: rs_consul::acl_types::create_a_c_l_token_payload_builder::State>
+pub struct rs_consul::CreateOrUpdateKeyRequestBuilder<'a, S: rs_consul::types::create_or_update_key_request_builder::State>
+pub struct rs_consul::DeleteKeyRequestBuilder<'a, S: rs_consul::types::delete_key_request_builder::State>
+pub struct rs_consul::DeregisterEntityPayloadBuilder<S: rs_consul::types::deregister_entity_payload_builder::State>
+pub struct rs_consul::GetNodesRequestBuilder<'a, S: rs_consul::types::get_nodes_request_builder::State>
+pub struct rs_consul::GetServiceNodesRequestBuilder<'a, S: rs_consul::types::get_service_nodes_request_builder::State>
+pub struct rs_consul::LockRequestBuilder<'a, S: rs_consul::types::lock_request_builder::State>
+pub struct rs_consul::LockWatchRequestBuilder<'a, S: rs_consul::types::lock_watch_request_builder::State>
+pub struct rs_consul::ReadKeyRequestBuilder<'a, S: rs_consul::types::read_key_request_builder::State>
+pub struct rs_consul::RegisterEntityCheckBuilder<S: rs_consul::types::register_entity_check_builder::State>
+pub struct rs_consul::RegisterEntityPayloadBuilder<S: rs_consul::types::register_entity_payload_builder::State>
+pub struct rs_consul::RegisterEntityServiceBuilder<S: rs_consul::types::register_entity_service_builder::State>

Checks

Please check these off before promoting the pull request to non-draft status.

  • All CI checks are green.
  • I have reviewed the proposed changes myself.

Copy link

github-actions bot commented Jul 2, 2025

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

Copy link
Contributor

@kushudai kushudai left a comment

Choose a reason for hiding this comment

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

Hey @x0rw Did you consider bon instead of a custom macro?
A little hesitant to ship a proc macro crate on top of the current one to crates.io
See https://bon-rs.com/guide/alternatives

@x0rw
Copy link
Contributor Author

x0rw commented Jul 10, 2025

Hey @kushudai
Yes i had the same concern about nested crates, and though about turning the root into a workspace but it still felt heavy, since we don't have have any custom behaviour that would requires our own builder, i think switching to bon makes sense here, if we ever need custom logic down the line, we can always introduce a seperate proc-macro crate later.
what do you think?.

@kushudai
Copy link
Contributor

Hey @kushudai Yes i had the same concern about nested crates, and though about turning the root into a workspace but it still felt heavy, since we don't have have any custom behaviour that would requires our own builder, i think switching to bon makes sense here, if we ever need custom logic down the line, we can always introduce a seperate proc-macro crate later. what do you think?.

Sounds great!
I looked at the deps in bon and it seems lightweight
https://github.com/elastio/bon/blob/9abdd3debef090608325c6541d409f8cebde8ca9/bon/Cargo.toml#L44-L49C1

@x0rw
Copy link
Contributor Author

x0rw commented Jul 10, 2025

Yeah, i compared it with derive_builder, bon is faster and lighter,

@kushudai
Copy link
Contributor

@x0rw I merged a nit. I'm happy to merge this whenever you're ready.
I can get a new release out next week

@x0rw
Copy link
Contributor Author

x0rw commented Jul 18, 2025

Hey @kushudai .

Hey, I've been busy the past week, I really slept on this PR 😅 --- Let me do another manual review tomorrow and update the PR description, then we can get this merged 👍🏻.

@kushudai
Copy link
Contributor

Hey @kushudai .

Hey, I've been busy the past week, I really slept on this PR 😅 --- Let me do another manual review tomorrow and update the PR description, then we can get this merged 👍🏻.

Thank you, no worries at all 😄

@x0rw
Copy link
Contributor Author

x0rw commented Jul 19, 2025

Hey @kushudai .
This is ready to merge.

@kushudai kushudai merged commit cd04dc7 into Roblox:main Jul 20, 2025
4 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants