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.

Sign up for GitHub

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

Jump to bottom

Drop whipper caching #336

Merged
merged 6 commits into from
Sep 17, 2020
Merged

Drop whipper caching #336

merged 6 commits into from
Sep 17, 2020

Conversation

JoeLametta
Copy link
Collaborator

@JoeLametta JoeLametta commented Nov 21, 2018 •
edited
Loading

Whipper's caching implementation causes a few issues (#196, #230, #321 (comment)) and complicates the code: it's better to drop this feature.

This fixes #335, fixes #196 and fixes #230.

@JoeLametta JoeLametta force-pushed the bugfix/issue-335-drop-caching branch from a2eec4c to be954c8 Compare November 21, 2018 17:02
Freso
Freso requested changes Nov 21, 2018
View reviewed changes
Copy link
Member

@Freso Freso left a comment

Choose a reason for hiding this comment

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

I love how much code this gets rid of. Seems like it will make the code simpler indeed. One small wording (docstring) change and please also just get rid of the cache_path() stuff. It's not used anywhere anymore according to git grep.

I haven't tested this though, just reviewed it "by eye", so it may well break everything. 😂 I'll try and find some time to run a rip using this branch.

whipper/common/accurip.py Outdated Show resolved Hide resolved
whipper/common/directory.py Outdated Show resolved Hide resolved
whipper/common/program.py Outdated Show resolved Hide resolved
whipper/test/test_common_directory.py Outdated Show resolved Hide resolved
@JoeLametta JoeLametta force-pushed the bugfix/issue-335-drop-caching branch from be954c8 to 8c918b6 Compare November 22, 2018 08:31
@JoeLametta
Copy link
Collaborator Author

Regarding to the rip resume thing I think it may work without caching too but the logfile is going to miss some information about the tracks which have been already ripped.

  1:
    Filename: /path/to/track.flac
    Pre-gap length: 00:00:50
    Peak level: 0.1234567
    Pre-emphasis: No
    Extraction speed: 1.2 X
    Extraction quality: 100.00 %
    Test CRC: 0CFA7ABF
    Copy CRC: 0CFA7ABF
    AccurateRip v1:
        Result: Found, exact match
        Confidence: 9
        Local CRC: CD2E3B53
        Remote CRC: CD2E3B53
    AccurateRip v2:
        Result: Found, exact match
        Confidence: 19
        Local CRC: 3D05ABCF
        Remote CRC: 3D05ABCF
    Status: Copy OK

@JoeLametta JoeLametta force-pushed the bugfix/issue-335-drop-caching branch 2 times, most recently from 18f971c to ab4e7f4 Compare December 14, 2018 21:28
@JoeLametta JoeLametta force-pushed the bugfix/issue-335-drop-caching branch from ab4e7f4 to e8712e5 Compare January 18, 2019 09:58
@JoeLametta JoeLametta force-pushed the bugfix/issue-335-drop-caching branch 2 times, most recently from 1faa9de to 7009329 Compare February 2, 2019 20:15
@JoeLametta JoeLametta force-pushed the bugfix/issue-335-drop-caching branch 3 times, most recently from d109a43 to 68d4bb6 Compare November 7, 2019 10:54
@JoeLametta JoeLametta force-pushed the bugfix/issue-335-drop-caching branch from 68d4bb6 to 0a9e456 Compare November 26, 2019 20:11
@JoeLametta
Copy link
Collaborator Author

Rebased (fixed merge conflicts).

1 similar comment
@JoeLametta
Copy link
Collaborator Author

Rebased (fixed merge conflicts).

@JoeLametta JoeLametta force-pushed the bugfix/issue-335-drop-caching branch from 0a9e456 to 8f3c8c4 Compare November 27, 2019 18:34
@JoeLametta
Copy link
Collaborator Author

Fixed merge conflict.

@JoeLametta JoeLametta force-pushed the bugfix/issue-335-drop-caching branch from faed0a2 to 6c080f6 Compare February 22, 2020 18:20
@JoeLametta
Copy link
Collaborator Author

Fixed merge conflicts.

@JoeLametta
Copy link
Collaborator Author

Shall we revive this one?

@JoeLametta JoeLametta force-pushed the bugfix/issue-335-drop-caching branch from 6c080f6 to d4005bd Compare February 22, 2020 18:27
@jwflory
Copy link

jwflory commented Apr 16, 2020

It would be cool to see this get merged. 😄

@JoeLametta JoeLametta force-pushed the bugfix/issue-335-drop-caching branch from d4005bd to 2755227 Compare September 17, 2020 15:02
@JoeLametta
Remove caching item from TODO
7a41b27 
Signed-off-by: JoeLametta <JoeLametta@users.noreply.github.com>
@JoeLametta
Delete unneeded files related to caching
19f987a 
Signed-off-by: JoeLametta <JoeLametta@users.noreply.github.com>
@JoeLametta
Update 'common/directory.py' & 'test/test_common_directory.py' (cachi…
2ddb2ea 
…ng removal)

Signed-off-by: JoeLametta <JoeLametta@users.noreply.github.com>
@JoeLametta
Update 'common/accurip.py' & 'test/test_common_accurip.py' (caching r…
c58f54e 
…emoval)

Signed-off-by: JoeLametta <JoeLametta@users.noreply.github.com>
@JoeLametta
Update 'common/program.py' (caching removal)
cb5b3b7 
Signed-off-by: JoeLametta <JoeLametta@users.noreply.github.com>
@JoeLametta
Update 'command/cd.py' (caching removal)
7be8ca0 
Signed-off-by: JoeLametta <JoeLametta@users.noreply.github.com>
@JoeLametta JoeLametta force-pushed the bugfix/issue-335-drop-caching branch from 2755227 to 7be8ca0 Compare September 17, 2020 15:06
@JoeLametta JoeLametta changed the title WIP: Drop whipper caching Drop whipper caching Sep 17, 2020
@JoeLametta JoeLametta merged commit 3acc3ff into develop Sep 17, 2020
@JoeLametta JoeLametta deleted the bugfix/issue-335-drop-caching branch September 17, 2020 15:52
@JoeLametta JoeLametta mentioned this pull request Sep 17, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtl999 jtl999 jtl999 left review comments

@Freso Freso Freso requested changes

@MerlijnWajer MerlijnWajer Awaiting requested review from MerlijnWajer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@JoeLametta @jwflory @Freso @jtl999