Skip to content

Use enum to simplify the UPTransportZenohBuilder#138

Closed
evshary wants to merge 3 commits into
mainfrom
use_enum
Closed

Use enum to simplify the UPTransportZenohBuilder#138
evshary wants to merge 3 commits into
mainfrom
use_enum

Conversation

@evshary
Copy link
Copy Markdown
Contributor

@evshary evshary commented Nov 11, 2025

It will be easier to use an enum, and we don't need to declare the Zenoh default config explicitly.

Signed-off-by: ChenYing Kuo <evshary@gmail.com>
@evshary evshary requested a review from sophokles73 November 11, 2025 04:03
Signed-off-by: ChenYing Kuo <evshary@gmail.com>
Copy link
Copy Markdown
Contributor

@sophokles73 sophokles73 left a comment

Choose a reason for hiding this comment

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

This basically removes the Type State pattern altogether, doesn't it? With your changes, you can now (again) set any properties on the builder :-(

@evshary
Copy link
Copy Markdown
Contributor Author

evshary commented Nov 11, 2025

I guess I didn't get what you wanted to do before.
Do you mean, for example, when users use with_config, then they can't call with_config_path or with_session again?

@sophokles73
Copy link
Copy Markdown
Contributor

I guess I didn't get what you wanted to do before. Do you mean, for example, when users use with_config, then they can't call with_config_path or with_session again?

exactly :-)
see https://cliffle.com/blog/rust-typestate/

Signed-off-by: ChenYing Kuo <evshary@gmail.com>
@evshary
Copy link
Copy Markdown
Contributor Author

evshary commented Nov 12, 2025

How about this? Keep using the Type State, but reduce the number of types by using enum. I think it should be simpler.

Copy link
Copy Markdown
Contributor

@sophokles73 sophokles73 left a comment

Choose a reason for hiding this comment

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

Hmm, this seems to only make a tiny difference in code length but only as long as the ConfiguredBuilderState variants are basically immutable, i.e. there are no additional configuration properties supported on them. What I also liked about the previous approach was that we could implement the build function without match because we already now what we are dealing with ...

FMPOV this change does not really improve the code but only makes it less flexible and reduces encapsulation.

@evshary
Copy link
Copy Markdown
Contributor Author

evshary commented Nov 12, 2025

I see. FMPOV, using match instead of implementing multiple traits, is more readable and easier to understand.
Although it might be less flexible, I don't foresee we will have too many changes here in the future.
Readability is what I would like to improve.

Anyway, I don't think this change really matters and needs a lot of discussion. I will close the PR.

@evshary evshary closed this Nov 12, 2025
@evshary evshary deleted the use_enum branch November 12, 2025 08:43
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