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

WIP: Remove unused arguments for program.cdrdao.ReadTOCTask() #368

Closed
wants to merge 1 commit into from

Conversation

Freso
Copy link
Member

@Freso Freso commented Feb 13, 2019

The one place the class is currently instanced, neither fast_toc nor toc_path is used, so there's hardly any reason to keep those around.

The one place the class is currently instanced, neither `fast_toc` nor
`toc_path` is used, so there's hardly any reason to keep those around.

Signed-off-by: Frederik “Freso” S. Olesen <freso.dk@gmail.com>
@Freso Freso added the Improvement Minor improvement to code label Feb 13, 2019
@JoeLametta
Copy link
Collaborator

neither fast_toc nor toc_path is used

That seems to be an error (not what you've found, I mean the current state): fast_toc should be used. Right now getFastToc() executes cdrdao without the fast-toc switch, in the same way as getTable().

I've also found this:

def getTable(self, runner, cddbdiscid, mbdiscid, device, offset,
toc_path):
"""
Retrieve the Table from the drive.
@rtype: L{table.Table}
"""
itable = None
tdict = {}
t = cdrdao.ReadTOCTask(device)
t.description = "Reading table"
t.toc_path = toc_path

toc_path seems to be set but without being passed as an argument? (is that a wise choice?)

@JoeLametta JoeLametta requested review from JoeLametta and removed request for JoeLametta February 14, 2019 20:25
@JoeLametta JoeLametta changed the title Remove unused arguments for program.cdrdao.ReadTOCTask() WIP: Remove unused arguments for program.cdrdao.ReadTOCTask() Feb 14, 2019
@Freso
Copy link
Member Author

Freso commented Feb 14, 2019

fast_toc should be used.

Why? I've noted before that e.g., ISRC gathering becomes unreliable when using --fast-toc and @jtl999 also made a comment that they'd prefer not to use it.

toc_path seems to be set but without being passed as an argument

Ah, I did miss that I'll update this to no longer include the toc_path. Not using it as an argument is okay, I guess, except it makes it harder to grok instances of the class (like I did), since it won't show in a git grep for the class name.

@JoeLametta
Copy link
Collaborator

Why? I've noted before that e.g., ISRC gathering becomes unreliable when using --fast-toc and @jtl999 also made a comment that they'd prefer not to use it.

Didn't remember that. Anyway we're still performing the slow cdrdao read two times in a row. Maybe we can completely remove the getFastToc function and use only getTable: that will require updating both whipper/common/program.py and whipper/command/cd.py.

@mtdcr
Copy link
Contributor

mtdcr commented Sep 21, 2019

Unless there's a compelling reason against it, I'm going to submit a PR to make getFastToc() fast again. The result of getFastToc() only get's used to compute IDs for CDDB and MusicBrainz (and thus also an index into whipper's table cache). No ISRC data will ever get used from it.

My PR will also include a patch to convert the last other uses of ittoc to itable to let the limited purpose of getFastToc() appear more clearly.

@JoeLametta
Copy link
Collaborator

JoeLametta commented Oct 21, 2019

Closed because of #417 & 2e966a4.

@JoeLametta JoeLametta closed this Oct 21, 2019
JoeLametta added a commit that referenced this pull request Oct 21, 2019
Resolves #368.

Signed-off-by: JoeLametta <JoeLametta@users.noreply.github.com>
@JoeLametta JoeLametta deleted the task/cleanup-ReadTOCTask branch October 22, 2019 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Minor improvement to code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants