Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tentative first pass at making simulcast egest possible #312
base: master
Are you sure you want to change the base?
Tentative first pass at making simulcast egest possible #312
Changes from 10 commits
2742632
d4160e1
9fdf70b
ec670b4
4974cf4
379f403
21607ac
a3c2a20
d6ff71d
d4c27fc
19fc8e9
acef0fd
ac59df1
4b04180
246044e
850fcb5
e20c15d
866965e
25cedc0
2972dd5
55bc75c
8a59116
f780884
9707863
ffad0b5
6b23661
7853a8d
afe4dcb
b64f163
6a24250
fd84deb
f5857dd
5aa6864
6996968
7da6936
5007093
7114d5e
7c03a11
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think when comparing to an option
option = Some(value)
might be easier to read thanmap_or
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, you can't just do a
Some(value)
because the types don't line up, track.rid() isOption<String>
(I toyed with having it be something else, I don't know what the rust convention is and it seemed to match the other types across the codebase) and rid is a&str
, so I'd have to make it owned in order to do the comparison which seems even more of a stretch than doing amap_or
, which is fundamentally equivalent to the oft-usedfromMaybe
in a functional language like haskell/purescript.If the left hand type can be changed then this would go away. If you have a suggestion for this then please do tell (I did start with
&Option<String>
to avoid the clone but that just pushed the clone to the consumers which was 🤮 )There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do
track.rid().as_deref() == Some(rid)
but I guess that's not much better than this. By usingas_deref
the Deref coercion mechanism kicks in, resulting in a left hand side type ofOption<&str>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a neat little feature - would there be benefit in changing the type of track.rid()? It's mostly used in checks just like this anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point actually, it should be
Option<&str>
to avoid the allocation each time it's called. The caller can stillclone
it if they wantThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah okay,
Option<String>
on the struct andOption<&str>
on the trait is much nicer (and analogous to how you'd do a String anyway), I note that there are plenty of inconsistencies in how strings are passed around in the codebase in general and I've left most of them alone in this change so there are a few unnecessary String::from's here, but on the whole it's much nicer