3
\$\begingroup\$

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()

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

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

    parser.add_argument(
      '--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)

    reality = playlist.read()
    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)

        reality = playlist.read()
        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)

        reality = playlist.read()
        assert expectation == reality
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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? \$\endgroup\$ – Martin Kleiven Oct 15 '17 at 11:21
  • \$\begingroup\$ Lovely! I didn't know you could unpack lists! Didn't consider the option at all! \$\endgroup\$ – Martin Kleiven Oct 15 '17 at 17:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.