Repository: antonagestam/collectfast

Issue:
Breaking behavior 1.0.0 -> 1.3.0 (COLLECTSTATIC_STRATEGY)
Not sure if bug or intentional, but `collectstatic` command will break if you don't define `COLLECTSTATIC_STRATEGY` or `STATICFILES_STORAGE` in your settings file (even if `COLLECTFAST_ENABLED = False`). 

![image](https://user-images.githubusercontent.com/2257119/67443243-dbaf5800-f5b8-11e9-9edd-4c26b2364136.png)


Discussion:

Patch:
diff --git a/collectfast/__init__.py b/collectfast/__init__.py
index 67bc602..9c73af2 100644
--- a/collectfast/__init__.py
+++ b/collectfast/__init__.py
@@ -1,1 +1,1 @@
-__version__ = "1.3.0"
+__version__ = "1.3.1"
diff --git a/collectfast/management/commands/collectstatic.py b/collectfast/management/commands/collectstatic.py
index 040b4bd..d7a4433 100644
--- a/collectfast/management/commands/collectstatic.py
+++ b/collectfast/management/commands/collectstatic.py
@@ -2,6 +2,7 @@ from multiprocessing.dummy import Pool
 from typing import Any
 from typing import Dict
 from typing import List
+from typing import Optional
 from typing import Tuple
 from typing import Type
 
@@ -13,6 +14,7 @@ from django.core.management.base import CommandParser
 
 from collectfast import __version__
 from collectfast import settings
+from collectfast.strategies import DisabledStrategy
 from collectfast.strategies import guess_strategy
 from collectfast.strategies import load_strategy
 from collectfast.strategies import Strategy
@@ -27,7 +29,7 @@ class Command(collectstatic.Command):
         self.num_copied_files = 0
         self.tasks = []  # type: List[Task]
         self.collectfast_enabled = settings.enabled
-        self.strategy = self._load_strategy()(self.storage)
+        self.strategy = DisabledStrategy(Storage())  # type: Strategy
 
     @staticmethod
     def _load_strategy():
@@ -62,8 +64,11 @@ class Command(collectstatic.Command):
     def set_options(self, **options):
         # type: (Any) -> None
         """Set options and handle deprecation."""
-        disable = options.pop("disable_collectfast", False)
-        self.collectfast_enabled = not disable
+        self.collectfast_enabled = self.collectfast_enabled and not options.pop(
+            "disable_collectfast"
+        )
+        if self.collectfast_enabled:
+            self.strategy = self._load_strategy()(self.storage)
         super().set_options(**options)
 
     def collect(self):
@@ -73,14 +78,18 @@ class Command(collectstatic.Command):
         Command.copy_file() which is called by super().collect().
         """
         ret = super().collect()
+        if not self.collectfast_enabled:
+            return ret
         if settings.threads:
             Pool(settings.threads).map(self.maybe_copy_file, self.tasks)
         return ret
 
     def handle(self, *args, **options):
-        # type: (Any, Any) -> str
+        # type: (Any, Any) -> Optional[str]
         """Override handle to suppress summary output."""
-        super().handle(**options)
+        ret = super().handle(**options)
+        if not self.collectfast_enabled:
+            return ret
         return "{} static file{} copied.".format(
             self.num_copied_files, "" if self.num_copied_files == 1 else "s"
         )
@@ -90,9 +99,9 @@ class Command(collectstatic.Command):
         """Determine if file should be copied or not and handle exceptions."""
         path, prefixed_path, source_storage = args
 
-        self.strategy.pre_should_copy_hook()
-
         if self.collectfast_enabled and not self.dry_run:
+            self.strategy.pre_should_copy_hook()
+
             if not self.strategy.should_copy_file(path, prefixed_path, source_storage):
                 self.log("Skipping '%s'" % path)
                 return
@@ -107,7 +116,7 @@ class Command(collectstatic.Command):
         file with a blocking call.
         """
         args = (path, prefixed_path, source_storage)
-        if settings.threads:
+        if settings.threads and self.collectfast_enabled:
             self.tasks.append(args)
         else:
             self.maybe_copy_file(args)
diff --git a/collectfast/strategies/__init__.py b/collectfast/strategies/__init__.py
index 9d52a9d..4d93b5e 100644
--- a/collectfast/strategies/__init__.py
+++ b/collectfast/strategies/__init__.py
@@ -1,5 +1,6 @@
+from .base import DisabledStrategy
 from .base import guess_strategy
 from .base import load_strategy
 from .base import Strategy
 
-__all__ = ("load_strategy", "guess_strategy", "Strategy")
+__all__ = ("load_strategy", "guess_strategy", "Strategy", "DisabledStrategy")
diff --git a/collectfast/strategies/base.py b/collectfast/strategies/base.py
index 1705b3f..198e2d4 100644
--- a/collectfast/strategies/base.py
+++ b/collectfast/strategies/base.py
@@ -9,6 +9,7 @@ from functools import lru_cache
 from io import BytesIO
 from typing import ClassVar
 from typing import Generic
+from typing import NoReturn
 from typing import Optional
 from typing import Tuple
 from typing import Type
@@ -138,6 +139,16 @@ class CachingHashStrategy(HashStrategy[_RemoteStorage], abc.ABC):
         return str(file_hash)
 
 
+class DisabledStrategy(Strategy):
+    def should_copy_file(self, path, prefixed_path, local_storage):
+        # type: (str, str, Storage) -> NoReturn
+        raise NotImplementedError
+
+    def pre_should_copy_hook(self):
+        # type: () -> NoReturn
+        raise NotImplementedError
+
+
 def load_strategy(klass: Union[str, type, object]) -> Type[Strategy[Storage]]:
     if isinstance(klass, str):
         klass = pydoc.locate(klass)


Test patch:
diff --git a/collectfast/tests/command/__init__.py b/collectfast/tests/command/__init__.py
new file mode 100644
index 0000000..e69de29
diff --git a/collectfast/tests/test_command.py b/collectfast/tests/command/test_command.py
similarity index 76%
rename from collectfast/tests/test_command.py
rename to collectfast/tests/command/test_command.py
index c63908d..002f1a9 100644
--- a/collectfast/tests/test_command.py
+++ b/collectfast/tests/command/test_command.py
@@ -1,18 +1,17 @@
-from io import StringIO
-from typing import Any
 from unittest import TestCase
 
 from django.core.exceptions import ImproperlyConfigured
-from django.core.management import call_command
 from django.test import override_settings as override_django_settings
 
-from .utils import clean_static_dir
-from .utils import create_static_file
-from .utils import make_test
-from .utils import override_setting
-from .utils import override_storage_attr
-from .utils import test_many
+from .utils import call_collectstatic
 from collectfast.management.commands.collectstatic import Command
+from collectfast.tests.utils import clean_static_dir
+from collectfast.tests.utils import create_static_file
+from collectfast.tests.utils import make_test
+from collectfast.tests.utils import override_setting
+from collectfast.tests.utils import override_storage_attr
+from collectfast.tests.utils import test_many
+
 
 aws_backend_confs = {
     "boto3": override_django_settings(
@@ -43,15 +42,6 @@ make_test_aws_backends = test_many(**aws_backend_confs)
 make_test_all_backends = test_many(**all_backend_confs)
 
 
-def call_collectstatic(*args, **kwargs):
-    # type: (Any, Any) -> str
-    out = StringIO()
-    call_command(
-        "collectstatic", *args, verbosity=3, interactive=False, stdout=out, **kwargs
-    )
-    return out.getvalue()
-
-
 @make_test_all_backends
 def test_basics(case):
     # type: (TestCase) -> None
@@ -73,25 +63,6 @@ def test_threads(case):
     case.assertIn("0 static files copied.", call_collectstatic())
 
 
-@make_test
-@override_django_settings(
-    STATICFILES_STORAGE="django.contrib.staticfiles.storage.StaticFilesStorage"
-)
-def test_disable_collectfast_with_default_storage(case):
-    # type: (TestCase) -> None
-    clean_static_dir()
-    create_static_file()
-    case.assertIn("1 static file copied.", call_collectstatic(disable_collectfast=True))
-
-
-@make_test
-def test_disable_collectfast(case):
-    # type: (TestCase) -> None
-    clean_static_dir()
-    create_static_file()
-    case.assertIn("1 static file copied.", call_collectstatic(disable_collectfast=True))
-
-
 @make_test
 def test_dry_run(case):
     # type: (TestCase) -> None
diff --git a/collectfast/tests/command/test_disable.py b/collectfast/tests/command/test_disable.py
new file mode 100644
index 0000000..6a04820
--- /dev/null
+++ b/collectfast/tests/command/test_disable.py
@@ -0,0 +1,46 @@
+from unittest import mock
+from unittest import TestCase
+
+from django.test import override_settings as override_django_settings
+
+from .utils import call_collectstatic
+from collectfast.tests.utils import clean_static_dir
+from collectfast.tests.utils import create_static_file
+from collectfast.tests.utils import make_test
+from collectfast.tests.utils import override_setting
+
+
+@make_test
+@override_django_settings(
+    STATICFILES_STORAGE="django.contrib.staticfiles.storage.StaticFilesStorage"
+)
+def test_disable_collectfast_with_default_storage(case):
+    # type: (TestCase) -> None
+    clean_static_dir()
+    create_static_file()
+    case.assertIn("1 static file copied", call_collectstatic(disable_collectfast=True))
+
+
+@make_test
+def test_disable_collectfast(case):
+    # type: (TestCase) -> None
+    clean_static_dir()
+    create_static_file()
+    case.assertIn("1 static file copied.", call_collectstatic(disable_collectfast=True))
+
+
+@override_setting("enabled", False)
+@mock.patch("collectfast.management.commands.collectstatic.Command._load_strategy")
+def test_no_load_with_disable_setting(mocked_load_strategy):
+    # type: (mock.MagicMock) -> None
+    clean_static_dir()
+    call_collectstatic()
+    mocked_load_strategy.assert_not_called()
+
+
+@mock.patch("collectfast.management.commands.collectstatic.Command._load_strategy")
+def test_no_load_with_disable_flag(mocked_load_strategy):
+    # type: (mock.MagicMock) -> None
+    clean_static_dir()
+    call_collectstatic(disable_collectfast=True)
+    mocked_load_strategy.assert_not_called()
diff --git a/collectfast/tests/command/utils.py b/collectfast/tests/command/utils.py
new file mode 100644
index 0000000..64061e7
--- /dev/null
+++ b/collectfast/tests/command/utils.py
@@ -0,0 +1,13 @@
+from io import StringIO
+from typing import Any
+
+from django.core.management import call_command
+
+
+def call_collectstatic(*args, **kwargs):
+    # type: (Any, Any) -> str
+    out = StringIO()
+    call_command(
+        "collectstatic", *args, verbosity=3, interactive=False, stdout=out, **kwargs
+    )
+    return out.getvalue()