Skip to content

Keep builder state when a property is None - #11

Merged
adzialocha merged 2 commits into
p2panda:mainfrom
pojntfx:fix-builder
Jul 28, 2026
Merged

adzialocha merged 2 commits into
p2panda:mainfrom
pojntfx:fix-builder

Conversation

@pojntfx

@pojntfx pojntfx commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

With the current state on main, if you pass in a property as None, the whole builder gets overwritten. This PR changes that so in the case where a property is None, we just return the builder unchanged.

This fixes connecting to a bootstrap node - with the state on main, if you disable mDNS and connect via a bootstrap node, nothing ever connects otherwise since every None argument would wipe all preceding arguments, e.g. the bootstrap URL.

Heads up: My Rust is very rusty, so there might be a better way to do this I'm not aware of?

Signed-off-by: Felicitas Pojtinger <felicitas@pojtinger.com>
@adzialocha
adzialocha self-requested a review May 5, 2026 09:47

@adzialocha adzialocha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not a glib expert but I think this looks good!

From a Rust perspective you could do an early return for value right at the beginning before you go into the match statement, something like that:

let Some(value) = value else {
    return builder
};

Now you can assume that value will always be Some for all code coming after.

Signed-off-by: Felicitas Pojtinger <felicitas@pojtinger.com>
@pojntfx

pojntfx commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

@adzialocha Thanks for the review - I don't think I can do it before let builder = match pspec.name() { since I'm looking at whether each option is set or not, but I used the syntax you mentioned to make each individual arm a little bit more readable :)

Is there a chance we could get this merged some time soon? The bug is still there on main, my GObject bindings still have the bug as a result!

@adzialocha
adzialocha merged commit 07c41ab into p2panda:main Jul 28, 2026
1 check passed
@pojntfx

pojntfx commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Thanks a ton for the merge :) Updating my bindings now!

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