Merge pull request #1711 from diego-plan9/editor

editor: add unit tests, minor changes

beetsplug/edit.py

@@ -26,6 +26,7 @@ import difflib
 import yaml
 import collections
 import webbrowser
+from contextlib import nested
 from sys import exit
 from beets import config
 from beets import ui
@@ -265,6 +266,7 @@ class EditPlugin(plugins.BeetsPlugin):
                     with open(new.name) as f:
                         newyaml = f.read()
                         list(yaml.load_all(newyaml))
+                        os.remove(new.name)
                         break
                 except yaml.YAMLError as e:
                     print_(ui.colorize('text_warning',
@@ -279,6 +281,7 @@ class EditPlugin(plugins.BeetsPlugin):
 
             return newyaml, oldyaml
         else:
+            os.remove(new.name)
             exit()
 
     def get_editor(self, name):
@@ -301,8 +304,8 @@ class EditPlugin(plugins.BeetsPlugin):
             for o in newset:
                 for so in o:
                     ids = set((so[1].values()[0].split()))
-                    for id in ids:
-                        alld[id].update(
+                    for id_ in ids:
+                        alld[id_].update(
                             {so[0].items()[0][1]: so[1].items()[0][0]})
         else:
             for o in newset:
@@ -340,7 +343,8 @@ class EditPlugin(plugins.BeetsPlugin):
         self.save_write(changedob)
 
     def save_write(self, changedob):
-        if not ui.input_yn('really modify? (y/n)'):
+        if not ui.input_yn(ui.colorize('action_default',
+                                       'really modify? (y/n)')):
             return
 
         for ob in changedob:
@@ -409,30 +413,31 @@ class EditPlugin(plugins.BeetsPlugin):
         oldlines = oldfilestr.splitlines()
         diff = difflib.HtmlDiff()
         df = diff.make_file(newlines, oldlines)
-        ht = NamedTemporaryFile('w', suffix='.html', delete=False)
-        ht.write(df)
-        ht.flush()
-        hdn = ht.name
-        if not self.brw:
-            browser = os.getenv('BROWSER')
-            if browser:
-                os.system(browser + " " + hdn)
+
+        with NamedTemporaryFile('w', suffix='.html', bufsize=0) as ht:
+            # TODO: if webbrowser.open() is not blocking, ht might be deleted
+            # too soon - need to test
+            ht.write(df)
+            if not self.brw:
+                browser = os.getenv('BROWSER')
+                if browser:
+                    os.system(browser + " " + ht.name)
+                else:
+                    webbrowser.open(ht.name, new=2, autoraise=True)
             else:
-                webbrowser.open(hdn, new=2, autoraise=True)
-        else:
-            callmethod = [self.brw]
-            if self.brw_args:
-                callmethod.extend(self.brw_args)
-            callmethod.append(hdn)
-            subprocess.call(callmethod)
+                callmethod = [self.brw]
+                if self.brw_args:
+                    callmethod.extend(self.brw_args)
+                callmethod.append(ht.name)
+                subprocess.call(callmethod)
+
         return
 
     def vimdiff(self, newstringstr, oldstringstr):
-
-        newdiff = NamedTemporaryFile(suffix='.old.yaml', delete=False)
-        newdiff.write(newstringstr)
-        newdiff.close()
-        olddiff = NamedTemporaryFile(suffix='.new.yaml', delete=False)
-        olddiff.write(oldstringstr)
-        olddiff.close()
-        subprocess.call(['vimdiff', newdiff.name, olddiff.name])
+        with nested(NamedTemporaryFile(suffix='.old.yaml',
+                                       bufsize=0),
+                    NamedTemporaryFile(suffix='.new.yaml',
+                                       bufsize=0)) as (newdiff, olddiff):
+            newdiff.write(newstringstr)
+            olddiff.write(oldstringstr)
+            subprocess.call(['vimdiff', newdiff.name, olddiff.name])

docs/plugins/edit.rst

@@ -5,9 +5,9 @@ Add the ``edit`` plugin to your ``plugins:`` in your ``config.yaml``. Then
 you simply put in a query like you normally do.
 ::
 
-     beet yamleditor beatles
-     beet yamleditor beatles -a
-     beet yamleditor beatles -f '$title $lyrics'
+     beet edit beatles
+     beet edit beatles -a
+     beet edit beatles -f '$title $lyrics'
 
 
 

test/test_edit.py

@@ -0,0 +1,172 @@
+# This file is part of beets.
+# Copyright 2015, Adrian Sampson and Diego Moreda.
+#
+# Permission is hereby granted, free of charge, to any person obtaining
+# a copy of this software and associated documentation files (the
+# "Software"), to deal in the Software without restriction, including
+# without limitation the rights to use, copy, modify, merge, publish,
+# distribute, sublicense, and/or sell copies of the Software, and to
+# permit persons to whom the Software is furnished to do so, subject to
+# the following conditions:
+#
+# The above copyright notice and this permission notice shall be
+# included in all copies or substantial portions of the Software.
+
+from __future__ import (division, absolute_import, print_function,
+                        unicode_literals)
+import codecs
+
+from mock import Mock, patch
+from test._common import unittest
+from test.helper import TestHelper, control_stdin
+
+from beets import library
+from beetsplug.edit import EditPlugin
+
+
+class ModifyFileMocker(object):
+    """Helper for modifying a file, replacing or editing its contents. Used for
+    mocking the calls to the external editor during testing."""
+
+    def __init__(self, contents=None, replacements=None):
+        """ `self.contents` and `self.replacements` are initalized here, in
+        order to keep the rest of the functions of this class with the same
+        signature as `EditPlugin.get_editor()`, making mocking easier.
+            - `contents`: string with the contents of the file to be used for
+            `overwrite_contents()`
+            - `replacement`: dict with the in-place replacements to be used for
+            `replace_contents()`, in the form {'previous string': 'new string'}
+
+        TODO: check if it can be solved more elegantly with a decorator
+        """
+        self.contents = contents
+        self.replacements = replacements
+        self.action = self.overwrite_contents
+        if replacements:
+            self.action = self.replace_contents
+
+    def overwrite_contents(self, filename):
+        """Modify `filename`, replacing its contents with `self.contents`. If
+        `self.contents` is empty, the file remains unchanged.
+        """
+        if self.contents:
+            with codecs.open(filename, 'w', encoding='utf8') as f:
+                f.write(self.contents)
+
+    def replace_contents(self, filename):
+        """Modify `filename`, reading its contents and replacing the strings
+        specified in `self.replacements`.
+        """
+        with codecs.open(filename, 'r', encoding='utf8') as f:
+            contents = f.read()
+        for old, new_ in self.replacements.iteritems():
+            contents = contents.replace(old, new_)
+        with codecs.open(filename, 'w', encoding='utf8') as f:
+            f.write(contents)
+
+
+class EditCommandTest(unittest.TestCase, TestHelper):
+    """ Black box tests for `beetsplug.edit`. Command line interaction is
+    simulated using `test.helper.control_stdin()`, and yaml editing via an
+    external editor is simulated using `ModifyFileMocker`.
+    """
+    ALBUM_COUNT = 1
+    TRACK_COUNT = 10
+
+    def setUp(self):
+        self.setup_beets()
+        self.load_plugins('edit')
+        # make sure that we avoid invoking the editor except for making changes
+        self.config['edit']['diff_method'] = ''
+        # add an album
+        self.add_album_fixture(track_count=self.TRACK_COUNT)
+        # keep track of write()s
+        library.Item.write = Mock()
+
+    def tearDown(self):
+        self.teardown_beets()
+        self.unload_plugins()
+
+    def run_mocked_command(self, modify_file_args={}, stdin=[]):
+        """Run the edit command, with mocked stdin and yaml writing."""
+        m = ModifyFileMocker(**modify_file_args)
+        with patch.object(EditPlugin, 'get_editor', side_effect=m.action):
+            with control_stdin('\n'.join(stdin)):
+                self.run_command('edit')
+
+    def assertCounts(self, album_count=ALBUM_COUNT, track_count=TRACK_COUNT,
+                     write_call_count=TRACK_COUNT, title_starts_with=''):
+        """Several common assertions on Album, Track and call counts."""
+        self.assertEqual(len(self.lib.albums()), album_count)
+        self.assertEqual(len(self.lib.items()), track_count)
+        self.assertEqual(library.Item.write.call_count, write_call_count)
+        self.assertTrue(all(i.title.startswith(title_starts_with)
+                            for i in self.lib.items()))
+
+    def test_title_edit_discard(self):
+        """Edit title for all items in the library, then discard changes-"""
+        # edit titles
+        self.run_mocked_command({'replacements': {u't\u00eftle':
+                                                  u'modified t\u00eftle'}},
+                                # edit? y, done? y, modify? n
+                                ['y', 'y', 'n'])
+
+        self.assertCounts(write_call_count=0,
+                          title_starts_with=u't\u00eftle')
+
+    def test_title_edit_apply(self):
+        """Edit title for all items in the library, then apply changes."""
+        # edit titles
+        self.run_mocked_command({'replacements': {u't\u00eftle':
+                                                  u'modified t\u00eftle'}},
+                                # edit? y, done? y, modify? y
+                                ['y', 'y', 'y'])
+
+        self.assertCounts(write_call_count=self.TRACK_COUNT,
+                          title_starts_with=u'modified t\u00eftle')
+
+    def test_noedit(self):
+        """Do not edit anything."""
+        # do not edit anything
+        self.run_mocked_command({'contents': None},
+                                # edit? n -> "no changes found"
+                                ['n'])
+
+        self.assertCounts(write_call_count=0,
+                          title_starts_with=u't\u00eftle')
+
+    def test_malformed_yaml_discard(self):
+        """Edit the yaml file incorrectly (resulting in a malformed yaml
+        document), then discard changes.
+
+        TODO: this test currently fails, on purpose. User gets into an endless
+        "fix?" -> "ok.fixed." prompt loop unless he is able to provide a
+        well-formed yaml."""
+        # edit the yaml file to an invalid file
+        self.run_mocked_command({'contents': '!MALFORMED'},
+                                # edit? n, done? y, fix? n
+                                ['y', 'y', 'n'])
+
+        self.assertCounts(write_call_count=0,
+                          title_starts_with=u't\u00eftle')
+
+    def test_invalid_yaml_discard(self):
+        """Edit the yaml file incorrectly (resulting in a well-formed but
+        invalid yaml document), then discard changes.
+
+        TODO: this test currently fails, on purpose. `check_diff()` chokes
+        ungracefully"""
+        # edit the yaml file to an invalid file
+        self.run_mocked_command({'contents': 'wellformed: yes, but invalid'},
+                                # edit? n, done? y
+                                ['y', 'y'])
+
+        self.assertCounts(write_call_count=0,
+                          title_starts_with=u't\u00eftle')
+
+
+def suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)
+
+if __name__ == b'__main__':
+    unittest.main(defaultTest='suite')