Skip to content
This repository has been archived by the owner on Dec 8, 2024. It is now read-only.

Feature: Support MediaSegment@SeqId. #124

Merged
merged 1 commit into from
Feb 1, 2019
Merged

Conversation

leikao
Copy link
Collaborator

@leikao leikao commented Jan 29, 2019

Add MediaSegment.SeqId support.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 71.799% when pulling cea445a on leikao:master into f845940 on grafov:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 71.799% when pulling cea445a on leikao:master into f845940 on grafov:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 71.799% when pulling cea445a on leikao:master into f845940 on grafov:master.

@@ -251,6 +255,15 @@ func TestDecodeMediaPlaylist(t *testing.T) {
t.Errorf("Segment %v's title = %v (must = %q)", i, s.Title, titles[i])
}
}
if p.Count() != 522 {

Choose a reason for hiding this comment

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

Isn't better to use an assert here? I'm new to golang anyway

  assert.Equal(t, 522, p.Count(), "Expected segments quantity: 522.")

Choose a reason for hiding this comment

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

If yes I think there is lots of places where it can be asserted instead of conditional checked.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes Leandro, I use assert to write test cases in some of my projects, it seems good.
But the assert is not contained in Golang's official packages, I do not want to introduce a 3rd package for the time being, I hope keep this project to be simple.

Choose a reason for hiding this comment

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

Cool, I didn't know, thanks 👍

Choose a reason for hiding this comment

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

BTW, LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants