search

eclipse-uprotocol / up-rust

uProtocol Language Specific Library for Rust
Apache License 2.0
11 stars 9 forks source link

UMessageBuilder.with_comm_status() should use UCode as parameter value (instead of i32) #130

Closed AnotherDaniel closed 3 months ago

AnotherDaniel commented 3 months ago

Currently, UMessageBuilder.with_comm_status() is defined as follows:

`pub fn with_comm_status(&mut self, comm_status: i32) -> &mut UMessageBuilder;`

The UMessageBuilder comm_status member-type is UCode - and so the only thing that this method signature does is, it forces users to put in UCode::OK.value() (requiring the non-obvious extra import of protobuf::Enum), just so that with_comm_status() then directly translates the i32 back into a UCode.

So I propose to change this sig to just use comm_status: UCode. This is not going to take away any flexibility from us, as we are (these days) owning and maintaining our own definition of UCode (not using the outside Google ones any more)

Any objections @sophokles73 @evshary @PLeVasseur @stevenhartley ?

PLeVasseur commented 3 months ago

Makes sense. IIRC comm_status was changed to a UCode after the UMessageBuilder was written.

sophokles73 commented 3 months ago

Sounds reasonable to me :+1:

evshary commented 3 months ago

Sounds good to me.