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

Improve Text Chapters / YouTube import #1643

Merged
merged 2 commits into from
Jul 17, 2023
Merged

Conversation

thatrobotdev
Copy link
Contributor

@thatrobotdev thatrobotdev commented Jul 3, 2023

Starts fixing #1642, and allows timestamps with semicolons directly after them like 0:00: JDX - Tides (Trailerized) to be imported.

I have tested it with the two descriptions in #1642, but I'm not sure if this would unexpectedly break the regex or if there is a better way to do it.

@mifi
Copy link
Owner

mifi commented Jul 12, 2023

Hi. Why did you close this? I think it could be a useful improvement

@thatrobotdev
Copy link
Contributor Author

thatrobotdev commented Jul 14, 2023

Hi there! Thanks for the response. I closed it because I wasn't sure it fixed #1642 without causing any feature regressions, since I don't have the experience with testing or regex to make this change with confidence. It might work better as a draft PR, but I need some help with review and also the second description type that I added to the issue, which I think would require a more complex solution with timecodes out of order.

I've edited the PR to match its status better, thanks for the nudge!

@thatrobotdev thatrobotdev reopened this Jul 14, 2023
@thatrobotdev thatrobotdev marked this pull request as draft July 14, 2023 04:04
@thatrobotdev thatrobotdev changed the title Allows semicolons directly after timestamps in timestamp import Improve Text Chapters / YouTube import Jul 14, 2023
@mifi mifi marked this pull request as ready for review July 17, 2023 13:55
@mifi mifi merged commit 14d5c68 into mifi:master Jul 17, 2023
2 checks passed
@thatrobotdev
Copy link
Contributor Author

I appreciate you for reviewing and adding tests, thank you so much! :)

@mifi
Copy link
Owner

mifi commented Jul 19, 2023

Of course! thank you

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.

2 participants