Repository: facebookincubator/ptr

Issue:
Enable black for Python 3.7 once fixed
Thanks for logging an issue with `ptr`. Here are some questions to help try 
and pull out as much information possible to help us help you!

## How is `ptr` not doing what you expect?
Today, we don't run black with Python 3.7 on Windows

## What is your suggestion to make this better?
Once black works - enable it and remove hacks

## How can a developer reproduce this?
Run `black.exe` with Python 3.7 on Windows

- We could potentially also add a allowed to fail run on Azure if available ...

Blocked by https://github.com/ambv/black/issues/728

Discussion:

Patch:
diff --git a/ptr.py b/ptr.py
index a6172a1..2c0d434 100755
--- a/ptr.py
+++ b/ptr.py
@@ -427,20 +427,6 @@ async def _progress_reporter(
     LOG.debug("progress_reporter finished")
 
 
-# TODO: Remove when black works with Python 3.7 on Windows
-def _run_black(config: Dict, force_black: bool = False) -> bool:
-    """ Check if we're Windows and Python 3.7 or disabled via config
-       `black.exe` is broken with Python 3.7 - Issue Open:
-       https://github.com/ambv/black/issues/728 """
-    if not force_black and WINDOWS and sys.version_info >= (3, 7):
-        LOG.error(
-            "black is hard disabled due to not working with Python 3.7 on Windows"
-        )
-        return False
-
-    return bool("run_black" in config and config["run_black"])
-
-
 def _set_build_env(build_base_path: Optional[Path]) -> Dict[str, str]:
     build_environ = environ.copy()
     if not build_base_path or not build_base_path.exists():
@@ -496,7 +482,6 @@ async def _test_steps_runner(
     env: Dict,
     stats: Dict[str, int],
     print_cov: bool = False,
-    force_black: bool = False,
 ) -> Tuple[Optional[test_result], int]:
     bin_dir = "Scripts" if WINDOWS else "bin"
     exe = ".exe" if WINDOWS else ""
@@ -544,7 +529,7 @@ async def _test_steps_runner(
         ),
         step(
             StepName.black_run,
-            _run_black(config, force_black),
+            bool("run_black" in config and config["run_black"]),
             _generate_black_cmd(setup_py_path.parent, black_exe),
             "Running black for {}".format(setup_py_path),
             config["test_suite_timeout"],
@@ -648,7 +633,6 @@ async def _test_runner(
     test_results: List[test_result],
     venv_path: Path,
     print_cov: bool,
-    force_black: bool,
     stats: Dict[str, int],
     idx: int,
 ) -> None:
@@ -681,7 +665,6 @@ async def _test_runner(
             env,
             stats,
             print_cov,
-            force_black,
         )
         total_success_runtime = int(time() - test_run_start_time)
         if test_fail_result:
@@ -882,7 +865,6 @@ async def run_tests(
     venv_path: Optional[Path],
     venv_keep: bool,
     print_cov: bool,
-    force_black: bool,
     stats: Dict[str, int],
     stats_file: str,
     venv_timeout: float,
@@ -911,14 +893,7 @@ async def run_tests(
     test_results = []  # type: List[test_result]
     consumers = [
         _test_runner(
-            queue,
-            tests_to_run,
-            test_results,
-            venv_path,
-            print_cov,
-            force_black,
-            stats,
-            i + 1,
+            queue, tests_to_run, test_results, venv_path, print_cov, stats, i + 1
         )
         for i in range(atonce)
     ]
@@ -956,7 +931,6 @@ async def async_main(
     print_cov: bool,
     print_non_configured: bool,
     run_disabled: bool,
-    force_black: bool,
     stats_file: str,
     venv_timeout: float,
 ) -> int:
@@ -991,7 +965,6 @@ async def async_main(
         venv_path,
         venv_keep,
         print_cov,
-        force_black,
         stats,
         stats_file,
         venv_timeout,
@@ -1019,11 +992,6 @@ def main() -> None:
     parser.add_argument(
         "-d", "--debug", action="store_true", help="Verbose debug output"
     )
-    parser.add_argument(
-        "--force-black",
-        action="store_true",
-        help="Ensure black runs if enabled in config",
-    )
     parser.add_argument(
         "-k", "--keep-venv", action="store_true", help="Do not remove created venv"
     )
@@ -1086,7 +1054,6 @@ def main() -> None:
                     args.print_cov,
                     args.print_non_configured,
                     args.run_disabled,
-                    args.force_black,
                     args.stats_file,
                     args.venv_timeout,
                 )


Test patch:
diff --git a/ptr_tests.py b/ptr_tests.py
index fb8e02a..8d0bc00 100644
--- a/ptr_tests.py
+++ b/ptr_tests.py
@@ -13,7 +13,6 @@ from os import environ
 from pathlib import Path
 from shutil import rmtree
 from subprocess import CalledProcessError
-from sys import version_info
 from tempfile import TemporaryDirectory, gettempdir
 from typing import (  # noqa: F401 # pylint: disable=unused-import
     Any,
@@ -130,20 +129,7 @@ class TestPtr(unittest.TestCase):
     @patch("ptr.run_tests", async_none)
     @patch("ptr._get_test_modules")
     def test_async_main(self, mock_gtm: Mock) -> None:
-        args = [
-            1,
-            Path("/"),
-            "mirror",
-            1,
-            "venv",
-            True,
-            True,
-            False,
-            True,
-            False,
-            "stats",
-            30,
-        ]
+        args = [1, Path("/"), "mirror", 1, "venv", True, True, False, True, "stats", 30]
         mock_gtm.return_value = False
         self.assertEqual(
             self.loop.run_until_complete(ptr.async_main(*args)), 1  # pyre-ignore
@@ -382,17 +368,6 @@ class TestPtr(unittest.TestCase):
         )
         self.assertEqual(mock_log.call_count, 2)
 
-    def test_run_black(self) -> None:
-        config = {}  # type: Dict[str, Any]
-        self.assertFalse(ptr._run_black(config, False))
-        self.assertFalse(ptr._run_black(config, True))
-        config["run_black"] = True
-        if ptr.WINDOWS and version_info >= (3, 7):
-            # Ensure even if in config we don't run it
-            self.assertFalse(ptr._run_black(config, False))
-        else:
-            self.assertTrue(ptr._run_black(config, False))
-
     def test_set_build_env(self) -> None:
         local_build_path = Path(gettempdir())
         build_env = ptr._set_build_env(local_build_path)
@@ -429,7 +404,7 @@ class TestPtr(unittest.TestCase):
             stats = defaultdict(int)  # type: Dict[str, int]
             self.loop.run_until_complete(
                 ptr._test_runner(
-                    queue, tests_to_run, test_results, td_path, False, False, stats, 69
+                    queue, tests_to_run, test_results, td_path, False, stats, 69
                 )
             )
             self.assertEqual(len(test_results), 1)
@@ -464,14 +439,11 @@ class TestPtr(unittest.TestCase):
                 (None, 6) if ptr.WINDOWS else (None, 7),
             )
 
-            if ptr.WINDOWS and version_info >= (3, 7):
-                # No pyre + black
-                expected = (None, 6)
-            elif ptr.WINDOWS:
+            if ptr.WINDOWS:
                 # No pyre
                 expected = (None, 7)
             else:
-                # Running all steps on everything else
+                # Running all steps on all other platforms
                 expected = (None, 8)
 
             fake_tests_to_run = {fake_setup_py: ptr_tests_fixtures.EXPECTED_TEST_PARAMS}