Add constructors, setters, adders & builders#27
Conversation
|
an |
Constructors arguments include all non-optionals & non-collections attributes.
`String` arguments use `impl ToString`.
When only one argument is optional (e.g. `Attribute`), an additional constructor
`with_...` is implemented, otherwise setters / adders & builders are defined.
Adding an Attribute to a Session or a Media uses direct `impl ToString`
arguments so we can write:
```
media.add_attribute("attr");
media.add_attribute_with_value("attr", "value");
```
instead of:
```
media.add_attribute(Attribute::new("attr"));
media.add_attribute(Attribute::new_with_value("attr"));
```
Same for other builder-less argument types: `Connection`, `TimeZone` & `Key`.
| self | ||
| } | ||
|
|
||
| pub fn connection( |
There was a problem hiding this comment.
here and elsewhere I'm wondering why the builders don't take e.g. Connection directly here, but instead provide smaller setters
There was a problem hiding this comment.
Ah, I thought about mentioning it in the top comment, but then I forgot...
Connection, Attribute, Key and others have only two construction variants, so I thought it was more convenient and less verbose to include a direct setter in the builder. Ex:
.connection(sdp_types::ADDRTYPE_IP4, outbound_addr)instead of:
```rust
.connection(sdp_types::Connection::new(sdp_types::ADDRTYPE_IP4, outbound_addr))I could also use this for Origin btw.
There was a problem hiding this comment.
That makes sense but maybe a generic one based on the struct would also make sense (if you want to copy over a Connection from some other session or so)?
| } | ||
|
|
||
| /// Adds an "rtpmap" compliant attribute with the provided values. | ||
| pub fn rtpmap(mut self, pt: u8, encoding_name: impl AsRef<str>, clock_rate: u32) -> Self { |
There was a problem hiding this comment.
I wonder if this shouldn't be some kind of attribute builder
There was a problem hiding this comment.
As you wish. Related to previous comment.
There was a problem hiding this comment.
I don't mean that this should be removed but should probably be some kind of specific attribute builder instead
sdroege
left a comment
There was a problem hiding this comment.
Looks good to me otherwise, thanks!
|
What do you think about this?
|
Yes, see also #25 |
|
Maybe we should also add |
|
Just discussed with Tarun about my comment above. He already implemented the group attribute handling as part of this PR. In a separate PR, we could define some kind of view / lens mechanism where a user gets a version of the SDP filtered for a specific SSRC. |
|
Sounds good to me. Let me know (both of you) when you want either/both of the PRs merged. From my point of view this is all ready. |
This MR adds constructors, setters, adders & builders.
Constructors arguments include all non-optionals & non-collections attributes.
Stringarguments useimpl ToString.When only one argument is optional (e.g.
Attribute), an additional constructorwith_...is implemented, otherwise setters / adders & builders are defined.Adding an Attribute to a Session or a Media uses direct
impl ToStringarguments so we can write:instead of:
Same for other builder-less argument types:
Connection,TimeZone&Key.The second commit adds shortcuts to define the
rtpmapattribute.Example:
Fixes #5