-
Notifications
You must be signed in to change notification settings - Fork 78
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
Various improvements #29
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fuckin great, although i've had a few issues i would like to resolve with you
Hey @joshrmcdaniel thanks for the PR it looks good but it's quite a lot of changes for me to review and I'm not really able to put much time into this project. Would you be interested in taking over maintenance of this project? |
heya @joshrmcdaniel, i made some ajustments which may be useful for you to look at, namely: clearing the teriminl after each playlist
"fixing" the ratelimit
if you have discord, that would be a lot better to collaborate on this :) |
I can take a look at doing fixes this weekend @RobinHirst11 I don't mind taking over in regards to maintenance either @timrae |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to add some unit test cases for the matching algorithm. I'm a bit scared to change anything atm as there are no tests and I went to some effort to optimize the match rate. Unit tests would help to prevent regressions in the match rate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please split this into separate PRs? The first PR should be non-functional refactoring changes, and subsequent PRs should have functional changes. Currently it's very difficult to see what has been changed, particularly in the main sync file.
Would be great to incorporate some of these changes if you can pull them out into smaller PRs once #43 is merged |
@joshrmcdaniel Please email me if you're still interested in helping out with maintenance. You can find my email in the git history. |
This is currently a WIP, theres a couple of things TODO:
New features:
casefold
instead of lower, to better check title equality