Skip to content
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

Add support for additional values for the bandwidth type #100

Closed
wants to merge 2 commits into from

Conversation

babolivier
Copy link
Contributor

Description

Support additional bandwidth types defined in RFC3890 and RFC3556

Reference issue

Fixes #99

@codecov
Copy link

codecov bot commented Feb 18, 2021

Codecov Report

Merging #100 (a641650) into master (4f18d96) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #100   +/-   ##
=======================================
  Coverage   61.46%   61.46%           
=======================================
  Files          11       11           
  Lines         968      968           
=======================================
  Hits          595      595           
  Misses        271      271           
  Partials      102      102           
Flag Coverage Δ
go 61.46% <0.00%> (ø)
wasm 61.46% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
unmarshal.go 57.80% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f18d96...1f0ea2f. Read the comment docs.

@babolivier babolivier force-pushed the babolivier/bandwidth branch 3 times, most recently from f769a0f to 2751df9 Compare February 18, 2021 17:43
@Sean-Der
Copy link
Member

Nice! I would love to take this @babolivier mind adding a simple test?

Just something to be sure it doesn't accidently get regressed in the future :)

Support additional bandwidth types defined in RFC3890 and RFC3556
@babolivier
Copy link
Contributor Author

Nice! I would love to take this @babolivier mind adding a simple test?

Just something to be sure it doesn't accidently get regressed in the future :)

Makes total sense, I've added lines with these new types to MediaBandwidthSDP so they get passed through Marshal() in TestRoundTrip.

Also the CI is complaining about my commit message, but I can't see what's wrong with it. Am I required to add a body to the message?

@Sean-Der
Copy link
Member

Merged with f6cf15d

@Sean-Der Sean-Der closed this Feb 20, 2021
@Sean-Der
Copy link
Member

@babolivier yea the commit message linter is annoying. We put it in because we were getting really bad commit messages FIX ERROR kind of thing. I wish the tooling was better, someday will get to it :)

Since you are using pion/sdp (and may make more improvements) would you be interested in being added to the org? You can make branches and review PRs. Then you can merge your own stuff when another Pion dev approves (or you can approve others).

Makes things easier you can pin against branches and test your stuff. Also will give me someone to add to reviews in the future for SDP work :)

@babolivier
Copy link
Contributor Author

babolivier commented Feb 22, 2021

Since you are using pion/sdp (and may make more improvements) would you be interested in being added to the org? You can make branches and review PRs. Then you can merge your own stuff when another Pion dev approves (or you can approve others).

I'm sorry but while I'm using pion/sdp for what I'm working on right now (and I'm happy to open issues/PRs about issues or bugs I find) I'm not really interested in a more permanent involvement with the project 😅 Thanks for the offer though, and thanks for handling and merging this PR (as well as for the rest of your work on this project)!

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.

Support more values for the bandwidth types when parsing b= attributes
2 participants