Skip to content
This repository has been archived by the owner on Aug 6, 2022. It is now read-only.

Refactoring #2

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Refactoring #2

wants to merge 3 commits into from

Conversation

fogti
Copy link

@fogti fogti commented Dec 29, 2020

https://twitter.com/1kescher/status/1343964329952108546
unspaghetti'ed

actually, this PR modifies the code behavoir a bit, e.g. if a song isn't downloaded, because the destination file already exists, some debug output is skipped (e.g. moved after the condition), as this allows more efficient processing (but only works if the info wouldn't be interesting, anyways).

It is probably a good idea to squash this before merging (I think GitHub has this as an option).

@fogti fogti marked this pull request as ready for review December 29, 2020 20:15
Copy link

@fherrera124 fherrera124 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since commit 011a110, it stopped working

@fogti
Copy link
Author

fogti commented Dec 30, 2020

Ah ok, I did expect something like that, esp. as I wasn't really sure what that part of code was doing... I'll revert that part.
ok, fixed.

@fherrera124
Copy link

fherrera124 commented Jan 4, 2021

@zserik I added your commits to my fork and made some changes and added support to retrieve the covers, can you review it? If it's worth it, you can add some changes to this PR.
876b245
PS: I'm new to Rust, so if I made a change that I shouldn't have done, feel free to point me out.

shuffle some stmts around
move utility fns to the bottom
refac: move fname calculation into 'write_to_disk'
reorder code to improve clarity
get rid of duplicated file existence check
print invalid spotify types
handle_entry: get rid of unused generic lifetime params
cache Show and Album.name
@fogti
Copy link
Author

fogti commented Mar 7, 2021

I condensed the code into 3 commits, which should still capture most of the intent behind the changes. (I especially wanted to get rid of some back-and-forth stuff and couldn't move all commits into place without introducing conflicts while rebasing, so I squashed the long run).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants