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(Parser)!: general refactoring of parsers #344

Merged
merged 10 commits into from
Mar 15, 2023
Merged

Conversation

Wykerd
Copy link
Collaborator

@Wykerd Wykerd commented Mar 9, 2023

Description

General refactor of parsers to cleanup the codebase.

Breaking Changes

  • Removes PlaylistAuthor in favor of Author
  • Removes NavigatableText in favor of Text
  • YT.VideoInfo constructor changed to match those of YTKids and YTMusic

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes
  • I have checked my code and corrected any misspellings

@Wykerd Wykerd force-pushed the Wykerd-refactor-parser branch from 1308382 to e248ba5 Compare March 13, 2023 10:13
@Wykerd Wykerd marked this pull request as ready for review March 13, 2023 10:15
@absidue
Copy link
Collaborator

absidue commented Mar 14, 2023

You might have to rebase this pull request on top of the main branch so that you don't break #351. This pull request doesn't pass an Actions instance to the toDash function, which is needed to generate the DASH manifest for the OTF format streams.

@Wykerd
Copy link
Collaborator Author

Wykerd commented Mar 14, 2023

You might have to rebase this pull request on top of the main branch so that you don't break #351. This pull request doesn't pass an Actions instance to the toDash function, which is needed to generate the DASH manifest for the OTF format streams.

Fixed. Already did a merge, just forgot to update the toDash method of MediaInfo.

@absidue
Copy link
Collaborator

absidue commented Mar 14, 2023

Thanks for fixing that, also it's probably worth changing the start of the title from refactor(Parser) to refactor(Parser)! so that it release please marks it this as a breaking change in the changelog. Things like the constructor for Video info changed and the PlaylistAuthor class was removed, as both of those are exported from the library, it's entirely possible that they could be used in user's code, which will break with this change.

@absidue
Copy link
Collaborator

absidue commented Mar 14, 2023

https://github.com/googleapis/release-please#how-can-i-fix-release-notes alternatively you can add a section like this to the pull request description to override the message that release please uses. That way you could also list the specific things that are breaking changes (tried it in #351 but need to wait for release please to run again before I know if it worked)

@LuanRT
Copy link
Owner

LuanRT commented Mar 14, 2023

(tried it in #351 but need to wait for release please to run again before I know if it worked)

Seems like it did.

@Wykerd Wykerd changed the title refactor(Parser): general refactoring of parsers refactor(Parser)!: general refactoring of parsers Mar 15, 2023
@Wykerd
Copy link
Collaborator Author

Wykerd commented Mar 15, 2023

I just realized I already did merge NavigatableText and Text for #310 oops

@absidue
Copy link
Collaborator

absidue commented Mar 15, 2023

I just realized I already did merge NavigatableText and Text for #310 oops

You can do the commit override stuff after a pull request is merged too. Release-please digs through all pull requests merged since the last release every time it runs (when a commit is pushed to the main branch either directly or by a pull request getting merged)

@LuanRT LuanRT merged commit b13bf6e into main Mar 15, 2023
@Wykerd Wykerd deleted the Wykerd-refactor-parser branch March 16, 2023 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants