Rip out all code that uses gstreamer #130
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We can now rip CDs without gstreamer. This is not the most clean attempt, but I have tried to remove most of the code that depends on gstreamer. I hope there is not a lot of code left that depends on code that I have removed - I can at least rip a CD fully.
Freso
requested changes
Feb 9, 2017
There was a problem hiding this comment.
Okay, from a pure read-over, I don't really have any functional comments, just code style ones. Basically a bunch of comments about blank lines.
The old code wasn't PEP 8, but this cleanup might be a good chance to move a slight bit closer to it?
| @@ -59,8 +60,6 @@ def add_arguments(self): | |||
| ) | |||
|
|
|||
| def do(self): | |||
| # here to avoid import gst eating our options | |||
| from morituri.common import encode | |||
|
|
|||
There was a problem hiding this comment.
Maybe also remove this empty line?
| @@ -209,8 +210,6 @@ def _arcs(self, runner, table, track, offset): | |||
| track, offset) | |||
| runner.run(t) | |||
|
|
|||
| # here to avoid import gst eating our options | |||
| from morituri.common import checksum | |||
|
|
|||
There was a problem hiding this comment.
Maybe also remove this empty line?
| @@ -135,47 +135,6 @@ def formatTime(seconds, fractional=3): | |||
|
|
|||
| return " ".join(chunks) | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Next line is a class so this empty line should be kept. :)
| @@ -491,8 +490,6 @@ def getHTOA(self): | |||
| return (start, stop) | |||
|
|
|||
| def verifyTrack(self, runner, trackResult): | |||
| # here to avoid import gst eating our options | |||
| from morituri.common import checksum | |||
|
|
|||
There was a problem hiding this comment.
Probably want to remove this line too.
| @@ -135,8 +137,6 @@ def __init__(self, image): | |||
|
|
|||
| path = image.getRealPath(index.path) | |||
|
|
|||
| # here to avoid import gst eating our options | |||
| from morituri.common import checksum | |||
|
|
|||
There was a problem hiding this comment.
Probably want to remove this empty line.
| cue = image.cue | ||
| self._tasks = [] | ||
| self.lengths = {} | ||
|
|
||
| def add(index): | ||
| # here to avoid import gst eating our options | ||
| from morituri.common import encode | ||
|
|
JoeLametta
reviewed
Feb 12, 2017
There was a problem hiding this comment.
I'll repeat what I've written on IRC.
Human inspection
Overall the PR looks good, the only thing left to be done is updating the Travis CI YML file to remove GStreamer's dependencies.
Static/Dynamic inspection
Please note that some issues are probably not relevant to this PR:
morituri/command/cd.py
- Unused import statement (at line 34)
- Expected type 'str', got 'unicode' instead (at line 513)
morituri/command/debug.py
- Cannot find reference 'ALL_PROFILES' in 'encode.py' (at line 162)
- Cannot find reference 'ALL_PROFILES' in 'encode.py' (at line 171)
- Unresolved attribute reference 'peak' for class 'FlacEncodeTask' (at line 194)
- Cannot find reference 'TagReadTask' in 'encode.py' (at line 217)
- Expected type 'str', got 'unicode' instead (at line 245)
- Signature of method 'RCCue.do()' does not match signature of base method in class 'BaseCommand' (at line 37)
- Signature of method 'RCList.do()' does not match signature of base method in class 'BaseCommand' (at line 61)
- Signature of method 'RCLog.do()' does not match signature of base method in class 'BaseCommand' (at line 96)
morituri/command/image.py
- Unresolved attribute reference 'checksum' for class 'Task' (at line 148)
- Cannot find reference 'EncodeTaskFlac' in 'encode.py' (at line 240)
morituri/command/offset.py
- No module named BeautifulSoup (at line 9)
- Local variable 'prog' is assigned to but never used (at line 84)
morituri/common/checksum.py
- Unused import statement (at line 25)
- Unused import statement (at line 24)
- Unused import statement (at line 30)
- Unused import statement (at line 23)
morituri/common/encode.py
- Unused import statement (at line 30)
- Unused import statement (at line 24)
- Unused import statement (at line 31)
- Unused import statement (at line 23)
- Unused import statement (at line 25)
- Unused import statement (at line 26)
morituri/common/program.py
- Function 'ripTrack' does not have a parameter 'number' (at line 518)
- Function 'ripTrack' does not have a parameter 'number' (at line 519)
- Cannot find reference 'ImageRetagTask' in 'image.py' (at line 564)
morituri/image/image.py
- Cannot find reference 'SafeRetagTask' in 'encode.py' (at line 99)
|
I checked out Everything went smoothly, AFAICT. 👍 |
Freso
requested changes
Feb 13, 2017
|
@Freso Did you look at “ImportError: No module named pygtk 100 %”? It’s not related to this issue but appears in your log, so… |
|
@ArchangeGabriel Yes. But |
|
I have been extremely busy IRL, but will have time this weekend to review this. |
Partial fixHere's a patch which fixes all the previously reported warnings affecting the following files:
And two very low importance ones: morituri/common/program.py
diff --git a/morituri/command/cd.py b/morituri/command/cd.py
index 0e70abc..38e03e0 100644
--- a/morituri/command/cd.py
+++ b/morituri/command/cd.py
@@ -31,7 +31,6 @@ import gobject
gobject.threads_init()
from morituri.command.basecommand import BaseCommand
-from morituri.common import encode
from morituri.common import (
accurip, common, config, drive, program, task
)
@@ -510,7 +509,8 @@ Log files will log the path to tracks relative to this directory.
logger.debug('writing m3u file for %r', discName)
m3uPath = u'%s.m3u' % discName
handle = open(m3uPath, 'w')
- handle.write(u'#EXTM3U\n')
+ u = u'#EXTM3U\n'
+ handle.write(u.encode('utf-8'))
def writeFile(handle, path, length):
targetPath = common.getRelativePath(path, m3uPath)
diff --git a/morituri/command/offset.py b/morituri/command/offset.py
index ff5d0dd..065f2ea 100644
--- a/morituri/command/offset.py
+++ b/morituri/command/offset.py
@@ -81,7 +81,6 @@ CD in the AccurateRip database."""
logger.debug('Trying with offsets %r', self._offsets)
def do(self):
- prog = program.Program(config.Config())
runner = ctask.SyncRunner()
device = self.options.device
diff --git a/morituri/common/checksum.py b/morituri/common/checksum.py
index a63a702..9ed12a8 100644
--- a/morituri/common/checksum.py
+++ b/morituri/common/checksum.py
@@ -20,14 +20,10 @@
# You should have received a copy of the GNU General Public License
# along with morituri. If not, see <http://www.gnu.org/licenses/>.
-import os
-import struct
-import zlib
import binascii
import wave
-from morituri.common import common, task
from morituri.extern.task import task as etask
from morituri.program.arc import accuraterip_checksum
diff --git a/morituri/common/encode.py b/morituri/common/encode.py
index 83352a5..4d3b69a 100644
--- a/morituri/common/encode.py
+++ b/morituri/common/encode.py
@@ -20,15 +20,9 @@
# You should have received a copy of the GNU General Public License
# along with morituri. If not, see <http://www.gnu.org/licenses/>.
-import math
-import os
-import shutil
-import tempfile
from mutagen.flac import FLAC
-from morituri.common import common
-from morituri.common import task as ctask
from morituri.extern.task import task
from morituri.program import sox
diff --git a/morituri/common/program.py b/morituri/common/program.py
index 1dab168..d1285aa 100644
--- a/morituri/common/program.py
+++ b/morituri/common/program.py
@@ -515,8 +515,6 @@ class Program:
@param trackResult: the object to store information in.
@type trackResult: L{result.TrackResult}
- @param number: track number (1-based)
- @type number: int
"""
if trackResult.number == 0:
start, stop = self.getHTOA()Errata:
As we still need to think about "image mode" destiny, I propose to temporarily ignore the issues affecting it. Updated status:morituri/common/program.py
morituri/command/debug.py
|
|
Merge? |
|
Almost. :) |
Partial fixFixed (including the changes addressed by previous patch): morituri/command/debug.py
diff --git a/morituri/command/cd.py b/morituri/command/cd.py
index 0e70abc..38e03e0 100644
--- a/morituri/command/cd.py
+++ b/morituri/command/cd.py
@@ -31,7 +31,6 @@ import gobject
gobject.threads_init()
from morituri.command.basecommand import BaseCommand
-from morituri.common import encode
from morituri.common import (
accurip, common, config, drive, program, task
)
@@ -510,7 +509,8 @@ Log files will log the path to tracks relative to this directory.
logger.debug('writing m3u file for %r', discName)
m3uPath = u'%s.m3u' % discName
handle = open(m3uPath, 'w')
- handle.write(u'#EXTM3U\n')
+ u = u'#EXTM3U\n'
+ handle.write(u.encode('utf-8'))
def writeFile(handle, path, length):
targetPath = common.getRelativePath(path, m3uPath)
diff --git a/morituri/command/debug.py b/morituri/command/debug.py
index ee3b664..d286266 100644
--- a/morituri/command/debug.py
+++ b/morituri/command/debug.py
@@ -153,14 +153,6 @@ class Encode(BaseCommand):
# here to avoid import gst eating our options
from morituri.common import encode
- default = 'flac'
- # slated for deletion as flac will be the only encoder
- self.parser.add_argument('--profile',
- action="store",
- dest="profile",
- help="profile for encoding (default '%s', choices '%s')" % (
- default, "', '".join(encode.ALL_PROFILES.keys())),
- default=default)
self.parser.add_argument('input', action='store',
help="audio file to encode")
self.parser.add_argument('output', nargs='?', action='store',
@@ -168,7 +160,6 @@ class Encode(BaseCommand):
def do(self):
from morituri.common import encode
- profile = encode.ALL_PROFILES[self.options.profile]()
try:
fromPath = unicode(self.options.input)
@@ -180,7 +171,7 @@ class Encode(BaseCommand):
try:
toPath = unicode(self.options.output)
except IndexError:
- toPath = fromPath + '.' + profile.extension
+ toPath = fromPath + '.flac'
runner = task.SyncRunner()
diff --git a/morituri/command/offset.py b/morituri/command/offset.py
index ff5d0dd..065f2ea 100644
--- a/morituri/command/offset.py
+++ b/morituri/command/offset.py
@@ -81,7 +81,6 @@ CD in the AccurateRip database."""
logger.debug('Trying with offsets %r', self._offsets)
def do(self):
- prog = program.Program(config.Config())
runner = ctask.SyncRunner()
device = self.options.device
diff --git a/morituri/common/checksum.py b/morituri/common/checksum.py
index a63a702..9ed12a8 100644
--- a/morituri/common/checksum.py
+++ b/morituri/common/checksum.py
@@ -20,14 +20,10 @@
# You should have received a copy of the GNU General Public License
# along with morituri. If not, see <http://www.gnu.org/licenses/>.
-import os
-import struct
-import zlib
import binascii
import wave
-from morituri.common import common, task
from morituri.extern.task import task as etask
from morituri.program.arc import accuraterip_checksum
diff --git a/morituri/common/encode.py b/morituri/common/encode.py
index 83352a5..4d3b69a 100644
--- a/morituri/common/encode.py
+++ b/morituri/common/encode.py
@@ -20,15 +20,9 @@
# You should have received a copy of the GNU General Public License
# along with morituri. If not, see <http://www.gnu.org/licenses/>.
-import math
-import os
-import shutil
-import tempfile
from mutagen.flac import FLAC
-from morituri.common import common
-from morituri.common import task as ctask
from morituri.extern.task import task
from morituri.program import sox
diff --git a/morituri/common/program.py b/morituri/common/program.py
index 1dab168..d1285aa 100644
--- a/morituri/common/program.py
+++ b/morituri/common/program.py
@@ -515,8 +515,6 @@ class Program:
@param trackResult: the object to store information in.
@type trackResult: L{result.TrackResult}
- @param number: track number (1-based)
- @type number: int
"""
if trackResult.number == 0:
start, stop = self.getHTOA()Updated statusIssues which need to be fixedmorituri/command/debug.py
|
|
@JoeLametta: Awesome work, sorry to drop this for a few weeks like this. I suggest that I look at the EncodeTaskFlac and checksum attribute reference issues (the ones in image.py), and the issues in debug.py; then we can merge the PR. As I stated before, we do not support retagging at this point, I think. Are you using pylint for this static analysis? If so, do you think we can add that to travis? That'd be awesome. I will fix the last outstanding issues on the 21st of April (today), after sleep. Then we can merge it and move forward! |
|
I've pushed JoeLametta's patch to the remove-gstreamer branch. Going through other issues now. |
Patch by JoeLametta
Untested; but should work.
Untested.
|
Ready to be merged. Regarding the issues (excluding unrelated/unconfirmed ones): Ignored:morituri/command/image.py
morituri/common/program.py
Will (probably) be addressed later:morituri/command/debug.py
|
JoeLametta
added a commit
that referenced
this pull request
Apr 24, 2017
Updated README.md, CHANGELOG.md, .travis.yml
This was referenced Apr 26, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We can now rip CDs without gstreamer.
This is not the most clean attempt, but I have tried to remove most of the
code that depends on gstreamer. I hope there is not a lot of code left that
depends on code that I have removed - I can at least rip a CD fully.
Please review and make notes and check what we are removing. Then please figure out what we want to keep, what we need to re-implement, and so on.