Modularize youtube-transcript's fetchTranscript API#35
Open
rshmhrj wants to merge 8 commits intoKakulukian:masterfrom
Open
Modularize youtube-transcript's fetchTranscript API#35rshmhrj wants to merge 8 commits intoKakulukian:masterfrom
rshmhrj wants to merge 8 commits intoKakulukian:masterfrom
Conversation
- changed method signature from private to protected to enable testing
- extract each piece of work from fetchTranscript into separate functions - add class variables to store state during processing and making passing data between methods easier (started with only videoId and config, but extended it to include all key data processing outputs) - refactor static fetchTranscript to instantiate new `ytt` object and perform the same original processing steps, to keep original interface intact and backward compatible - add docstrings for each method - add tests for original static method using fetch and created mocks / mock data for testing other ways of getting data from urls -- didn't want to add `obsidian` as a dev dependency only for testing
- previous version didn't work properly since the static fetchTranscript was called and it reinstantiated the ytt object of type YoutubeTranscript (instead of the overridden / extended child ObsidianYoutubeTranscript)
Author
|
Hi @Kakulukian @mefengl @balmacefa -- any chance you can make some time to review this please? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Hi @Kakulukian @mefengl @balmacefa, here is an initial draft at refactoring to modularize the
fetchTranscriptmethod, which allows users to override specific parts -- related to #34.Using YTTranscript from node or from a server works fine, but in the Obsidian Plugin web-view context, we are getting CORS errors. These changes should allow me to do something like this:
So I can use the
obsidian.requestmodule to perform the fetch for thepageBodyand for thetranscriptBodyand continue to use the rest of the functionality as defined.Also took the opportunity to add a bunch of tests, as I didn't want to break anything (and to also get a better idea of how everything worked). I used the command
npm run testto execute all the tests.What do you all think?