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

Various improvements #29

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

joshrmcdaniel
Copy link
Collaborator

This is currently a WIP, theres a couple of things TODO:

  • hide 429 log message
  • improve searching further
  • update docs
  • remove redundant functions

 
New features:

  • Caching is now a singleton, all songs retrieved in all playlists will be cached during run.
  • Significantly improved runtime
  • Only run process map on results not cached
  • CLI package
  • tidalapi to 0.7.2
  • Add logging
  • Searching now uses casefold instead of lower, to better check title equality
  • Searching now uses all spotify artists (if multiple artists specified in Spotify)
  • Refactor
  • Type hint
  • Most YAML file attributes can be specified on the CLI
  • Various other improvements

@joshrmcdaniel joshrmcdaniel marked this pull request as ready for review February 4, 2024 14:51
Copy link
Contributor

@RobinHirst11 RobinHirst11 left a 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

@timrae
Copy link
Collaborator

timrae commented Apr 28, 2024

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?

@RobinHirst11
Copy link
Contributor

RobinHirst11 commented Apr 29, 2024

heya @joshrmcdaniel, i made some ajustments which may be useful for you to look at, namely:
sync.txt

clearing the teriminl after each playlist

  • print("\033[2J\033[H", end="", flush=True)

"fixing" the ratelimit

  • tidal_tracks = call_async_with_progress(tidal_search, spotify_tracks, task_description, config.get('subprocesses', 10), tidal_session=tidal_session)

if you have discord, that would be a lot better to collaborate on this :)

@joshrmcdaniel
Copy link
Collaborator Author

I can take a look at doing fixes this weekend @RobinHirst11

I don't mind taking over in regards to maintenance either @timrae

@timrae timrae closed this May 7, 2024
@timrae timrae reopened this May 7, 2024
@timrae timrae self-requested a review May 7, 2024 07:32
Copy link
Collaborator

@timrae timrae left a 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.

Copy link
Collaborator

@timrae timrae left a 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.

@timrae
Copy link
Collaborator

timrae commented May 27, 2024

Would be great to incorporate some of these changes if you can pull them out into smaller PRs once #43 is merged

@timrae
Copy link
Collaborator

timrae commented Jun 12, 2024

@joshrmcdaniel Please email me if you're still interested in helping out with maintenance. You can find my email in the git history.

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.

3 participants