Merge pull request #89 from MichaelAquilina/cleanup

Code clean up and refactoring

s4/cli.py

@@ -5,30 +5,24 @@
 import json
 import logging
 import os
-import shutil
-import subprocess
 import sys
-import tempfile
 from collections import defaultdict
 
 import boto3
 
 from clint.textui import colored
 
-from inotify_simple import INotify, flags
-
-try:
-    from os import scandir
-except ImportError:
-    from scandir import scandir
+from inotify_simple import flags
 
 from tabulate import tabulate
-import tqdm
 
 from s4 import VERSION
 from s4 import sync
 from s4 import utils
 from s4.clients import local, s3
+from s4.diff import show_diff
+from s4.inotify_recursive import INotifyRecursive
+from s4.progressbar import ProgressBar
 from s4.resolution import Resolution
 
 
@@ -67,53 +61,6 @@ def handle_conflict(key, action_1, client_1, action_2, client_2):
         return Resolution.get_resolution(key, action_2, client_1, client_2)
 
 
-def show_diff(client_1, client_2, key):
-    if shutil.which("diff") is None:
-        print('Missing required "diff" executable.')
-        print("Install this using your distribution's package manager")
-        return
-
-    if shutil.which("less") is None:
-        print('Missing required "less" executable.')
-        print("Install this using your distribution's package manager")
-        return
-
-    so1 = client_1.get(key)
-    data1 = so1.fp.read()
-    so1.fp.close()
-
-    so2 = client_2.get(key)
-    data2 = so2.fp.read()
-    so2.fp.close()
-
-    fd1, path1 = tempfile.mkstemp()
-    fd2, path2 = tempfile.mkstemp()
-    fd3, path3 = tempfile.mkstemp()
-
-    with open(path1, 'wb') as fp:
-        fp.write(data1)
-    with open(path2, 'wb') as fp:
-        fp.write(data2)
-
-    # This is a lot faster than the difflib found in python
-    with open(path3, 'wb') as fp:
-        subprocess.call([
-            'diff', '-u',
-            '--label', client_1.get_uri(key), path1,
-            '--label', client_2.get_uri(key), path2,
-        ], stdout=fp)
-
-    subprocess.call(['less', path3])
-
-    os.close(fd1)
-    os.close(fd2)
-    os.close(fd3)
-
-    os.remove(path1)
-    os.remove(path2)
-    os.remove(path3)
-
-
 def get_s3_client(target, aws_access_key_id, aws_secret_access_key, region_name):
     s3_uri = s3.parse_s3_uri(target)
     s3_client = boto3.client(
@@ -252,7 +199,7 @@ def set_config(config):
         os.makedirs(CONFIG_FOLDER_PATH)
 
     with open(CONFIG_FILE_PATH, 'w') as fp:
-        json.dump(config, fp, indent=4)
+        json.dump(config, fp)
 
 
 def get_clients(entry):
@@ -278,18 +225,6 @@ def get_sync_worker(entry):
     return sync.SyncWorker(client_1, client_2)
 
 
-class INotifyRecursive(INotify):
-    def add_watches(self, path, mask):
-        results = {}
-        results[self.add_watch(path, mask)] = path
-
-        for item in scandir(path):
-            if item.is_dir():
-                results.update(self.add_watches(item.path, mask))
-
-        return results
-
-
 def daemon_command(args, config, logger, terminator=lambda x: False):
     all_targets = list(config['targets'].keys())
     if not args.targets:
@@ -344,30 +279,8 @@ def daemon_command(args, config, logger, terminator=lambda x: False):
             worker.sync(conflict_choice=args.conflicts)
 
 
-class ProgressBar(object):
-    """
-    Singleton wrapper around tqdm
-    """
-    pbar = None
-
-    @classmethod
-    def set_progress_bar(cls, *args, **kwargs):
-        if cls.pbar:
-            cls.pbar.close()
-
-        cls.pbar = tqdm.tqdm(*args, **kwargs)
-
-    @classmethod
-    def update(cls, value):
-        cls.pbar.update(value)
-
-    @classmethod
-    def hide(cls):
-        cls.pbar.close()
-
-
 def display_progress_bar(sync_object):
-    ProgressBar.set_progress_bar(
+    ProgressBar(
         total=sync_object.total_size,
         leave=False,
         ncols=80,
@@ -382,7 +295,7 @@ def update_progress_bar(value):
 
 
 def hide_progress_bar(sync_object):
-    ProgressBar.hide()
+    ProgressBar.close()
 
 
 def sync_command(args, config, logger):
@@ -450,9 +363,6 @@ def action_callback(resolution):
 
 
 def targets_command(args, config, logger):
-    if 'targets' not in config:
-        return
-
     for name in sorted(config['targets']):
         entry = config['targets'][name]
         print('{}: [{} <=> {}]'.format(name, entry['local_folder'], entry['s3_uri']))

s4/diff.py

@@ -0,0 +1,53 @@
+#! -*- encoding: utf8 -*-
+
+import os
+import shutil
+import subprocess
+import tempfile
+
+
+def show_diff(client_1, client_2, key):
+    if shutil.which("diff") is None:
+        print('Missing required "diff" executable.')
+        print("Install this using your distribution's package manager")
+        return
+
+    if shutil.which("less") is None:
+        print('Missing required "less" executable.')
+        print("Install this using your distribution's package manager")
+        return
+
+    so1 = client_1.get(key)
+    data1 = so1.fp.read()
+    so1.fp.close()
+
+    so2 = client_2.get(key)
+    data2 = so2.fp.read()
+    so2.fp.close()
+
+    fd1, path1 = tempfile.mkstemp()
+    fd2, path2 = tempfile.mkstemp()
+    fd3, path3 = tempfile.mkstemp()
+
+    with open(path1, 'wb') as fp:
+        fp.write(data1)
+    with open(path2, 'wb') as fp:
+        fp.write(data2)
+
+    # This is a lot faster than the difflib found in python
+    with open(path3, 'wb') as fp:
+        subprocess.call([
+            'diff', '-u',
+            '--label', client_1.get_uri(key), path1,
+            '--label', client_2.get_uri(key), path2,
+        ], stdout=fp)
+
+    subprocess.call(['less', path3])
+
+    os.close(fd1)
+    os.close(fd2)
+    os.close(fd3)
+
+    os.remove(path1)
+    os.remove(path2)
+    os.remove(path3)

s4/inotify_recursive.py

@@ -0,0 +1,20 @@
+#! -*- encoding: utf8 -*-
+
+try:
+    from os import scandir
+except ImportError:
+    from scandir import scandir
+
+from inotify_simple import INotify
+
+
+class INotifyRecursive(INotify):
+    def add_watches(self, path, mask):
+        results = {}
+        results[self.add_watch(path, mask)] = path
+
+        for item in scandir(path):
+            if item.is_dir():
+                results.update(self.add_watches(item.path, mask))
+
+        return results

s4/progressbar.py

@@ -0,0 +1,24 @@
+#! -*- encoding: utf-8 -*-
+
+import tqdm
+
+
+class ProgressBar(object):
+    """
+    Singleton wrapper around tqdm
+    """
+    pbar = None
+
+    def __new__(cls, *args, **kwargs):
+        if cls.pbar:
+            cls.pbar.close()
+
+        cls.pbar = tqdm.tqdm(*args, **kwargs)
+
+    @classmethod
+    def update(cls, value):
+        cls.pbar.update(value)
+
+    @classmethod
+    def close(cls):
+        cls.pbar.close()

tests/test_cli.py

@@ -140,75 +140,6 @@ def test_diff(self, show_diff, get_input, s3_client, local_client):
         show_diff.assert_called_with(s3_client, local_client, 'movie')
 
 
-class TestShowDiff(object):
-    @mock.patch('shutil.which')
-    def test_diff_not_found(self, which, capsys, local_client, s3_client):
-        which.return_value = None
-        cli.show_diff(local_client, s3_client, "something")
-
-        out, err = capsys.readouterr()
-        assert out == (
-            'Missing required "diff" executable.\n'
-            "Install this using your distribution's package manager\n"
-        )
-
-    @mock.patch('shutil.which')
-    def test_less_not_found(self, which, capsys, local_client, s3_client):
-        def missing_less(value):
-            return None if value == 'less' else 'something'
-
-        which.side_effect = missing_less
-        cli.show_diff(local_client, s3_client, "something")
-
-        out, err = capsys.readouterr()
-        assert out == (
-            'Missing required "less" executable.\n'
-            "Install this using your distribution's package manager\n"
-        )
-
-    @mock.patch('subprocess.call')
-    def test_diff(self, call, local_client, s3_client):
-        utils.set_local_contents(local_client, "something", 4000, "wow")
-        utils.set_s3_contents(s3_client, "something", 3000, "nice")
-
-        cli.show_diff(local_client, s3_client, "something")
-
-        assert call.call_count == 2
-        assert call.call_args_list[0][0][0][0] == "diff"
-        assert call.call_args_list[1][0][0][0] == "less"
-
-
-class TestINotifyRecursive(object):
-    @pytest.mark.timeout(5)
-    def test_add_watches(self, tmpdir):
-        foo = tmpdir.mkdir("foo")
-        bar = tmpdir.mkdir("bar")
-        baz = bar.mkdir("baz")
-
-        notifier = cli.INotifyRecursive()
-        result_1 = notifier.add_watches(str(foo), flags.CREATE)
-        result_2 = notifier.add_watches(str(bar), flags.CREATE)
-
-        assert sorted(result_1.values()) == sorted([str(foo)])
-        assert sorted(result_2.values()) == sorted([str(bar), str(baz)])
-
-        bar.join("hello.txt").write("hello")
-        foo.join("fennek.md").write("*jumps*")
-        baz.mkdir("bong")
-
-        events = notifier.read()
-        assert len(events) == 3
-
-        assert events[0].name == 'hello.txt'
-        assert result_2[events[0].wd] == str(bar)
-
-        assert events[1].name == 'fennek.md'
-        assert result_1[events[1].wd] == str(foo)
-
-        assert events[2].name == 'bong'
-        assert result_2[events[2].wd] == str(baz)
-
-
 def test_progressbar_smoketest(s3_client, local_client):
     # Just test that nothing blows up
     utils.set_local_contents(
@@ -903,7 +834,7 @@ def test_show_all(self, s3_client, local_client, capsys):
 class TestTargetsCommand(object):
 
     def test_empty(self, capsys):
-        cli.targets_command(None, {}, create_logger())
+        cli.targets_command(None, {'targets': {}}, create_logger())
         out, err = capsys.readouterr()
         assert out == err == ''
 

tests/test_diff.py

@@ -0,0 +1,44 @@
+#! -*- encoding: utf8 -*-
+
+import mock
+
+from s4 import diff
+from tests import utils
+
+
+class TestShowDiff(object):
+    @mock.patch('shutil.which')
+    def test_diff_not_found(self, which, capsys, local_client, s3_client):
+        which.return_value = None
+        diff.show_diff(local_client, s3_client, "something")
+
+        out, err = capsys.readouterr()
+        assert out == (
+            'Missing required "diff" executable.\n'
+            "Install this using your distribution's package manager\n"
+        )
+
+    @mock.patch('shutil.which')
+    def test_less_not_found(self, which, capsys, local_client, s3_client):
+        def missing_less(value):
+            return None if value == 'less' else 'something'
+
+        which.side_effect = missing_less
+        diff.show_diff(local_client, s3_client, "something")
+
+        out, err = capsys.readouterr()
+        assert out == (
+            'Missing required "less" executable.\n'
+            "Install this using your distribution's package manager\n"
+        )
+
+    @mock.patch('subprocess.call')
+    def test_diff(self, call, local_client, s3_client):
+        utils.set_local_contents(local_client, "something", 4000, "wow")
+        utils.set_s3_contents(s3_client, "something", 3000, "nice")
+
+        diff.show_diff(local_client, s3_client, "something")
+
+        assert call.call_count == 2
+        assert call.call_args_list[0][0][0][0] == "diff"
+        assert call.call_args_list[1][0][0][0] == "less"