Separate utils.read_file and read_file_binary

See #4467

8 files changed, 53 insertions, 48 deletions

diff --git a/qutebrowser/browser/pdfjs.py b/qutebrowser/browser/pdfjs.py
index 3bde1b8a4..5656fbfc2 100644
--- a/qutebrowser/browser/pdfjs.py
+++ b/qutebrowser/browser/pdfjs.py
@@ -149,7 +149,7 @@ def get_pdfjs_res_and_path(path):
     if content is None:
         res_path = '3rdparty/pdfjs/{}'.format(path)
         try:
-            content = utils.read_file(res_path, binary=True)
+            content = utils.read_file_binary(res_path)
         except FileNotFoundError:
             raise PDFJSNotFound(path) from None
         except OSError as e:
diff --git a/qutebrowser/browser/qutescheme.py b/qutebrowser/browser/qutescheme.py
index 3f95dae32..612bccdc0 100644
--- a/qutebrowser/browser/qutescheme.py
+++ b/qutebrowser/browser/qutescheme.py
@@ -270,7 +270,7 @@ def qute_javascript(url: QUrl) -> _HandlerRet:
     path = url.path()
     if path:
         path = "javascript" + os.sep.join(path.split('/'))
-        return 'text/html', utils.read_file(path, binary=False)
+        return 'text/html', utils.read_file(path)
     else:
         raise UrlInvalidError("No file specified")
 
@@ -371,7 +371,7 @@ def qute_help(url: QUrl) -> _HandlerRet:
     path = 'html/doc/{}'.format(urlpath)
     if not urlpath.endswith('.html'):
         try:
-            bdata = utils.read_file(path, binary=True)
+            bdata = utils.read_file_binary(path)
         except OSError as e:
             raise SchemeOSError(e)
         mimetype = utils.guess_mimetype(urlpath)
@@ -574,7 +574,7 @@ def qute_resource(url: QUrl) -> _HandlerRet:
     path = url.path().lstrip('/')
     mimetype = utils.guess_mimetype(path, fallback=True)
     try:
-        data = utils.read_file(path, binary=True)
+        data = utils.read_file_binary(path)
     except FileNotFoundError as e:
         raise NotFoundError(str(e))
     return mimetype, data
diff --git a/qutebrowser/utils/jinja.py b/qutebrowser/utils/jinja.py
index 4eaa53ead..59e887126 100644
--- a/qutebrowser/utils/jinja.py
+++ b/qutebrowser/utils/jinja.py
@@ -120,7 +120,7 @@ class Environment(jinja2.Environment):
 
     def _data_url(self, path: str) -> str:
         """Get a data: url for the broken qutebrowser logo."""
-        data = utils.read_file(path, binary=True)
+        data = utils.read_file_binary(path)
         mimetype = utils.guess_mimetype(path)
         return urlutils.data_url(mimetype, data).toString()
 
diff --git a/qutebrowser/utils/utils.py b/qutebrowser/utils/utils.py
index 483a9a6f3..18654749b 100644
--- a/qutebrowser/utils/utils.py
+++ b/qutebrowser/utils/utils.py
@@ -33,6 +33,7 @@ import contextlib
 import posixpath
 import shlex
 import mimetypes
+import pathlib
 import ctypes
 import ctypes.util
 from typing import (Any, Callable, IO, Iterator, Optional, Sequence, Tuple, Type, Union,
@@ -179,9 +180,22 @@ def compact_text(text: str, elidelength: int = None) -> str:
     return out
 
 
+def _resource_path(filename: str) -> pathlib.Path:
+    """Get a pathlib.Path object for a resource."""
+    assert not posixpath.isabs(filename), filename
+    assert os.path.pardir not in filename.split(posixpath.sep), filename
+
+    if hasattr(sys, 'frozen'):
+        # For PyInstaller, where we can't store resource files in a qutebrowser/ folder
+        # because the executable is already named "qutebrowser" (at least on macOS).
+        return pathlib.Path(sys.executable).parent / filename
+
+    return importlib_resources.files(qutebrowser) / filename
+
+
 def preload_resources() -> None:
     """Load resource files into the cache."""
-    resource_path = importlib_resources.files(qutebrowser)
+    resource_path = _resource_path('')
     for subdir, pattern in [('html', '*.html'), ('javascript', '*.js')]:
         path = resource_path / subdir
         for full_path in path.glob(pattern):
@@ -189,42 +203,33 @@ def preload_resources() -> None:
             _resource_cache[sub_path] = read_file(sub_path)
 
 
-# FIXME:typing Return value should be bytes/str
-def read_file(filename: str, binary: bool = False) -> Any:
+def read_file(filename: str) -> str:
     """Get the contents of a file contained with qutebrowser.
 
     Args:
         filename: The filename to open as string.
-        binary: Whether to return a binary string.
-                If False, the data is UTF-8-decoded.
 
     Return:
         The file contents as string.
     """
-    assert not posixpath.isabs(filename), filename
-    assert os.path.pardir not in filename.split(posixpath.sep), filename
-
-    if not binary and filename in _resource_cache:
+    if filename in _resource_cache:
         return _resource_cache[filename]
 
-    if hasattr(sys, 'frozen'):
-        # For PyInstaller, where we can't store resource files in a qutebrowser/ folder
-        # because the executable is already named "qutebrowser" (at least on macOS).
-        fn = os.path.join(os.path.dirname(sys.executable), filename)
-        if binary:
-            f: IO
-            with open(fn, 'rb') as f:
-                return f.read()
-        else:
-            with open(fn, 'r', encoding='utf-8') as f:
-                return f.read()
-    else:
-        p = importlib_resources.files(qutebrowser) / filename
+    path = _resource_path(filename)
+    return path.read_text(encoding='utf-8')
+
 
-        if binary:
-            return p.read_bytes()
+def read_file_binary(filename: str) -> bytes:
+    """Get the contents of a binary file contained with qutebrowser.
 
-        return p.read_text()
+    Args:
+        filename: The filename to open as string.
+
+    Return:
+        The file contents as a bytes object.
+    """
+    path = _resource_path(filename)
+    return path.read_bytes()
 
 
 def parse_version(version: str) -> VersionNumber:
diff --git a/tests/unit/browser/test_pdfjs.py b/tests/unit/browser/test_pdfjs.py
index 2a23faa73..788209d6f 100644
--- a/tests/unit/browser/test_pdfjs.py
+++ b/tests/unit/browser/test_pdfjs.py
@@ -77,7 +77,7 @@ class TestResources:
 
     @pytest.fixture
     def read_file_mock(self, mocker):
-        return mocker.patch.object(pdfjs.utils, 'read_file', autospec=True)
+        return mocker.patch.object(pdfjs.utils, 'read_file_binary', autospec=True)
 
     def test_get_pdfjs_res_system(self, read_system_mock):
         read_system_mock.return_value = (b'content', 'path')
diff --git a/tests/unit/browser/test_qutescheme.py b/tests/unit/browser/test_qutescheme.py
index a471dbfba..213df4e0c 100644
--- a/tests/unit/browser/test_qutescheme.py
+++ b/tests/unit/browser/test_qutescheme.py
@@ -44,9 +44,8 @@ class TestJavascriptHandler:
     @pytest.fixture(autouse=True)
     def patch_read_file(self, monkeypatch):
         """Patch utils.read_file to return few fake JS files."""
-        def _read_file(path, binary=False):
+        def _read_file(path):
             """Faked utils.read_file."""
-            assert not binary
             for filename, content in self.js_files:
                 if path == os.path.join('javascript', filename):
                     return content
@@ -158,13 +157,16 @@ class TestHelpHandler:
     @pytest.fixture
     def data_patcher(self, monkeypatch):
         def _patch(path, data):
-            def _read_file(name, binary=False):
+            def _read_file(name):
                 assert path == name
-                if binary:
-                    return data
                 return data.decode('utf-8')
 
+            def _read_file_binary(name):
+                assert path == name
+                return data
+
             monkeypatch.setattr(qutescheme.utils, 'read_file', _read_file)
+            monkeypatch.setattr(qutescheme.utils, 'read_file_binary', _read_file_binary)
         return _patch
 
     def test_unknown_file_type(self, data_patcher):
diff --git a/tests/unit/utils/test_jinja.py b/tests/unit/utils/test_jinja.py
index b62529eb3..5555560bf 100644
--- a/tests/unit/utils/test_jinja.py
+++ b/tests/unit/utils/test_jinja.py
@@ -34,30 +34,29 @@ from qutebrowser.config import configexc
 @pytest.fixture(autouse=True)
 def patch_read_file(monkeypatch):
     """pytest fixture to patch utils.read_file."""
-    def _read_file(path, binary=False):
+    def _read_file(path):
         """A read_file which returns a simple template if the path is right."""
         if path == os.path.join('html', 'test.html'):
-            assert not binary
             return """Hello {{var}}"""
         elif path == os.path.join('html', 'test2.html'):
-            assert not binary
             return """{{ resource_url('utils/testfile') }}"""
         elif path == os.path.join('html', 'test3.html'):
-            assert not binary
             return """{{ data_url('testfile.txt') }}"""
-        elif path == 'testfile.txt':
-            assert binary
-            return b'foo'
         elif path == os.path.join('html', 'undef.html'):
-            assert not binary
             return """{{ does_not_exist() }}"""
         elif path == os.path.join('html', 'attributeerror.html'):
-            assert not binary
             return """{{ obj.foobar }}"""
         else:
-            raise IOError("Invalid path {}!".format(path))
+            raise OSError("Invalid path {}!".format(path))
+
+    def _read_file_binary(path):
+        if path == 'testfile.txt':
+            return b'foo'
+        else:
+            raise OSError("Invalid path {}!".format(path))
 
     monkeypatch.setattr(jinja.utils, 'read_file', _read_file)
+    monkeypatch.setattr(jinja.utils, 'read_file_binary', _read_file_binary)
 
 
 def test_simple_template():
diff --git a/tests/unit/utils/test_utils.py b/tests/unit/utils/test_utils.py
index 7f89d2d4d..ee4cfe1e5 100644
--- a/tests/unit/utils/test_utils.py
+++ b/tests/unit/utils/test_utils.py
@@ -135,8 +135,7 @@ class TestReadFile:
 
     def test_readfile_binary(self):
         """Read a test file in binary mode."""
-        content = utils.read_file(os.path.join('utils', 'testfile'),
-                                  binary=True)
+        content = utils.read_file_binary(os.path.join('utils', 'testfile'))
         assert content.splitlines()[0] == b"Hello World!"