# A command-line program for managing music

I recently made a post of my program, now modified, here's for round two. Hopefully everything is actually okay by now, I'm just seeking validation, although I for one think it's ready to ship, heh. But please prove me wrong.

There are some things left from the last answer I got, I have them in the back of my head, but for now I'm well past good without them.

## mutil.py

The trickery with closures for the CustomAction classes lets me avoid having to define unique classes for every single feature supported by the program. All considered I think it's a good solution.

def main():
parser = init_parser()
parser.parse_args()

def make_remove_playlist_duplicates_action(command):
class CustomAction(argparse.Action):
def __call__(self, parser, namespace, values, option_string=None):
playlist_path = values[0]
with open(playlist_path) as playlist:
return command(playlist)
return CustomAction

def make_playlist_path_action(command):
class CustomAction(argparse.Action):
def __call__(self, parser, namespace, values, option_string=None):
playlist_path = values[0]
library_path = values[1]
with open(playlist_path) as playlist:
return command(playlist, library_path)
return CustomAction

def init_parser():
"""Initiate argparse and return an argparse.ArgumentParser
Returns: An ArgumentParser
"""
parser = argparse.ArgumentParser()

'--remove-duplicates', '-d',
nargs=1,
action=make_remove_playlist_duplicates_action(
commands.remove_playlist_duplicates)
)

'--use-absolute-paths', '-A',
nargs=2,
action=make_playlist_path_action(
commands.playlist_paths_use_absolute)
)

'--use-relative-paths', '-R',
nargs=2,
action=make_playlist_path_action(
commands.playlist_paths_use_relative)
)

return parser


## commands.py

These are the "commands" so to say supported by the program. I decided it's for the better if the user supplies the correct parameters in playlist_paths_use_absolute, instead of adding much complicated checking, I just raise an error. Screw the user. Other than that, mostly fast forward here, I hope.

def remove_playlist_duplicates(playlist):
"""Remove entries in a given playlist.
Args:
playlist: A file containing a playlist to check.

Returns: The number of duplicates removed.
"""
old_playlist = [entry.strip('\n') for entry in playlist]
unique_entries = list(OrderedDict.fromkeys(old_playlist))
new_playlist = '\n'.join(unique_entries)
util.overwrite_and_reset(playlist, new_playlist)

duplicates = len(old_playlist) - len(unique_entries)
return duplicates

def playlist_paths_use_relative(playlist, library_path):
"""Modify the playlist format to use relative paths.
Args:
playlist: A file-object containing a playlist to check.
library_path: The path in which your library resides
"""
util.process_playlist(lambda entry: entry.strip(library_path), playlist)

def playlist_paths_use_absolute(playlist, library_path):
"""Modify the playlist format to use absolute paths.
Args:
playlist: A file-object containing a playlist to check.
library_path: The path in which your library resides, must end in a '/'.
Raises:
ValueError: On incorrect library_path
"""
if library_path[-1:] != '/':
raise ValueError("library_path must include a trailing slash '/'")
util.process_playlist(lambda entry: library_path + entry if library_path not in entry else entry, playlist)


## util.py

    def process_playlist(f, playlist):
"""Process a playlist applying a function to each entry
Args:
f: The function to apply to playlist entries
playlist: The playlist to process
*args, **kwargs: Additional arguments to be passed on to f
"""
formatted_playlist = [entry.strip('\n') for entry in playlist]

new_playlist = '\n'.join(f(entry) for entry in formatted_playlist)
overwrite_and_reset(playlist, new_playlist)

def overwrite_and_reset(file, content):
"""Rewrite a file with content and then seek back to the start"""
file.seek(0)
file.truncate()
write_and_reset(file, content)

def write_and_reset(file, content):
file.write(content)
file.seek(0)


## test_commands.py

Can I do anything to reduce duplication further? This is 10 test-cases in total, parametrized.

@pytest.fixture
def playlist(fs):
"""Open a fake file object
Args:
fs: A reference to the pyfakefs filesystem
"""
fs.CreateFile('playlist')
with open('playlist', 'r+') as playlist:
yield playlist

@pytest.mark.parametrize('test_input, expectation, duplicates_expected', [
('a/b/c\na/b/c\n1/2/3', 'a/b/c\n1/2/3', 1),  # Duplicated
('a/b/c\n1/2/3', 'a/b/c\n1/2/3', 0),         # Non-duplicated
('', '', 0),                                 # Empty
])
def test_remove_playlist_duplicates(playlist, test_input, expectation, duplicates_expected):
util.write_and_reset(playlist, test_input)

duplicates_found = commands.remove_playlist_duplicates(playlist)

assert duplicates_expected == duplicates_found
assert expectation == reality

class TestPlaylistPathCommands:
library_path = '/home/$USER/Music/' library_path_w_trailing_slash = library_path + '/' absolute_path = '/home/$USER/Music/album/track'
relative_path = 'album/track'

@pytest.mark.parametrize('test_input, expectation', [
(absolute_path, relative_path),  # Absolute to relative
(relative_path, relative_path)   # Relative to relative
])
@pytest.mark.parametrize('library_path', [library_path, library_path_w_trailing_slash])
def test_playlist_paths_use_relative(self, playlist, test_input, expectation, library_path):
util.write_and_reset(playlist, test_input)

commands.playlist_paths_use_relative(playlist, library_path)

assert expectation == reality

@pytest.mark.parametrize('test_input, expectation', [
(relative_path, absolute_path),  # Relative to absolute
(absolute_path, absolute_path)   # Absolute to absolute
])
def test_playlist_paths_use_absolute(self, playlist, test_input, expectation):
util.write_and_reset(playlist, test_input)

commands.playlist_paths_use_absolute(playlist, self.library_path)

assert expectation == reality


I don't like this part of your code very much:

def make_remove_playlist_duplicates_action(command):
class CustomAction(argparse.Action):
def __call__(self, parser, namespace, values, option_string=None):
playlist_path = values[0]
with open(playlist_path) as playlist:
return command(playlist)
return CustomAction

def make_playlist_path_action(command):
class CustomAction(argparse.Action):
def __call__(self, parser, namespace, values, option_string=None):
playlist_path = values[0]
library_path = values[1]
with open(playlist_path) as playlist:
return command(playlist, library_path)
return CustomAction


First, CustomAction is not a descriptive name, and it's reused for (though only slightly) different behaviors.

But, more "serious", neither of these functions actually does what it says: The first is not creating / making a "remove playlist duplicates" action, and neither is the second creating / making a "playlist path action".

They are creating an action that takes (one or two) values from the parser, opens the first as a file, and then runs some given function with the file (and the remaining argument, for the second function).

So, they're one the one hand far more general than their names try to tell, and on the other hand IMO they do "too much" by doing two unrelated things: one, feeding values from the parser to a custom function. Two, opening a file.

Thus, I'd probably try to make a make_custom_action_from_function function that passes all values from the parser on to a custom function, and then either handle the file opening in the custom function(s) (probably not a good idea, DRY) or make a wrapper function that takes care of it.

I was thinking about something like this:

def make_custom_action(command):
class CustomAction(argparse.Action):
def __call__(self, parser, namespace, values, option_string=None):
return command(*values)
return CustomAction


Live example here. Though it's of course only a quick mockup, I didn't test for any drawbacks.

• So what you're saying is I should defer the responsibilities of my make_* functions to a single make_custom_action and then let that call another function(s) that deals with input processing? I can't see a way to make it work that way, I don't have control over what may be in values, and if I need to check for this in the 'custom function(s)' that would introduce a lot complexity and concerns. I do agree with the merit of the idea though, I need to make a single make_custom_action function with a general responsibility somehow. Can you please provide an example if I'm mistaken? Oct 15 '17 at 11:21
• Lovely! I didn't know you could unpack lists! Didn't consider the option at all! Oct 15 '17 at 17:10