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

refactor: clean up parameters of excludePlaylist #1304

Merged
merged 3 commits into from
Aug 19, 2022

Conversation

gesinger
Copy link
Contributor

@gesinger gesinger commented Aug 3, 2022

Rename the parameters passed to excludePlaylist to make the logic easier to follow.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors

@codecov
Copy link

codecov bot commented Aug 3, 2022

Codecov Report

Merging #1304 (c6b54bb) into next (206f099) will increase coverage by 0.00%.
The diff coverage is 94.41%.

@@           Coverage Diff           @@
##             next    #1304   +/-   ##
=======================================
  Coverage   86.27%   86.27%           
=======================================
  Files          39       39           
  Lines        9791     9795    +4     
  Branches     2275     2277    +2     
=======================================
+ Hits         8447     8451    +4     
  Misses       1344     1344           
Impacted Files Coverage Δ
src/videojs-http-streaming.js 90.45% <84.90%> (+0.04%) ⬆️
src/dash-playlist-loader.js 90.05% <87.34%> (ø)
src/playlist-loader.js 95.00% <95.23%> (-0.03%) ⬇️
src/manifest.js 93.45% <100.00%> (ø)
src/media-groups.js 98.66% <100.00%> (ø)
src/playback-watcher.js 98.33% <100.00%> (ø)
src/playlist-controller.js 94.98% <100.00%> (ø)
src/playlist-selectors.js 87.05% <100.00%> (ø)
src/playlist.js 94.56% <100.00%> (ø)
src/rendition-mixin.js 94.73% <100.00%> (ø)
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

One q around a comment. Otherwise, LGTM

Comment on lines 1149 to 1152
// If the `error` was generated by the playlist loader, it will contain
// the playlist we were trying to load (but failed) and that should be
// excluded instead of the currently selected playlist which is likely
// out-of-date in this scenario
Copy link
Member

Choose a reason for hiding this comment

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

is this comment still correct? At the very least, we won't be using the error.playlist here anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! I'll remove the whole comment.

Rename the parameters passed to excludePlaylist to make the logic
easier to follow.
@misteroneill misteroneill force-pushed the refactor/exclude-playlist branch from f383b29 to a689041 Compare August 19, 2022 14:50
@misteroneill
Copy link
Member

Rebased for @gesinger to resolve conflicts due to master/main renaming.

@misteroneill misteroneill merged commit ca3162b into videojs:next Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants