Repository: pallets/click
Pull request #1298 (2019-05-06 16:29:10)

click.open_file respects "errors" parameter.
Fixes https://github.com/pallets/click/issues/958

Signed-off-by: Joe Ledger <joewledger@gmail.com>

joewledger: Fixes https://github.com/pallets/click/issues/958

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 39cbd1d..dcf97d0 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -40,8 +40,6 @@ Unreleased
     ``type=bool``. :issue:`1287`
 -   Newlines in option help text are replaced with spaces before
     re-wrapping to avoid uneven line breaks. :issue:`834`
--   ``MissingParameter`` exceptions are printable in the Python
-    interpreter. :issue:`1139`
 
 
 Version 7.0
diff --git a/click/_compat.py b/click/_compat.py
index 982a265..f8efb29 100644
--- a/click/_compat.py
+++ b/click/_compat.py
@@ -329,80 +329,88 @@ else:
         # but this at least will force Click to recover somehow.
         return is_ascii_encoding(getattr(stream, 'encoding', None) or 'ascii')
 
-    def _is_compatible_text_stream(stream, encoding, errors):
-        stream_encoding = getattr(stream, 'encoding', None)
-        stream_errors = getattr(stream, 'errors', None)
+    def _is_compat_stream_attr(stream, attr, value):
+        """A stream attribute is compatible if it is equal to the
+        desired value or the desired value is unset and the attribute
+        has a value.
+        """
+        stream_value = getattr(stream, attr, None)
+        return stream_value == value or (value is None and stream_value is not None)
 
-        # Perfect match.
-        if stream_encoding == encoding and stream_errors == errors:
-            return True
+    def _is_compatible_text_stream(stream, encoding, errors):
+        """Check if a stream's encoding and errors attributes are
+        compatible with the desired values.
+        """
+        return (
+            _is_compat_stream_attr(stream, "encoding", encoding)
+            and _is_compat_stream_attr(stream, "errors", errors)
+        )
 
-        # Otherwise, it's only a compatible stream if we did not ask for
-        # an encoding.
-        if encoding is None:
-            return stream_encoding is not None
+    def _force_correct_text_stream(
+        text_stream,
+        encoding,
+        errors,
+        is_binary,
+        find_binary,
+        force_readable=False,
+        force_writable=False,
+    ):
+        if is_binary(text_stream, False):
+            binary_reader = text_stream
+        else:
+            # If the stream looks compatible, and won't default to a
+            # misconfigured ascii encoding, return it as-is.
+            if (
+                _is_compatible_text_stream(text_stream, encoding, errors)
+                and not (
+                    encoding is None
+                    and _stream_is_misconfigured(text_stream)
+                )
+            ):
+                return text_stream
 
-        return False
+            # Otherwise, get the underlying binary reader.
+            binary_reader = find_binary(text_stream)
 
-    def _force_correct_text_reader(text_reader, encoding, errors,
-                                   force_readable=False):
-        if _is_binary_reader(text_reader, False):
-            binary_reader = text_reader
-        else:
-            # If there is no target encoding set, we need to verify that the
-            # reader is not actually misconfigured.
-            if encoding is None and not _stream_is_misconfigured(text_reader):
-                return text_reader
-
-            if _is_compatible_text_stream(text_reader, encoding, errors):
-                return text_reader
-
-            # If the reader has no encoding, we try to find the underlying
-            # binary reader for it.  If that fails because the environment is
-            # misconfigured, we silently go with the same reader because this
-            # is too common to happen.  In that case, mojibake is better than
-            # exceptions.
-            binary_reader = _find_binary_reader(text_reader)
+            # If that's not possible, silently use the original reader
+            # and get mojibake instead of exceptions.
             if binary_reader is None:
-                return text_reader
+                return text_stream
 
-        # At this point, we default the errors to replace instead of strict
-        # because nobody handles those errors anyways and at this point
-        # we're so fundamentally fucked that nothing can repair it.
-        if errors is None:
-            errors = 'replace'
-        return _make_text_stream(binary_reader, encoding, errors,
-                                 force_readable=force_readable)
-
-    def _force_correct_text_writer(text_writer, encoding, errors,
-                                   force_writable=False):
-        if _is_binary_writer(text_writer, False):
-            binary_writer = text_writer
-        else:
-            # If there is no target encoding set, we need to verify that the
-            # writer is not actually misconfigured.
-            if encoding is None and not _stream_is_misconfigured(text_writer):
-                return text_writer
-
-            if _is_compatible_text_stream(text_writer, encoding, errors):
-                return text_writer
-
-            # If the writer has no encoding, we try to find the underlying
-            # binary writer for it.  If that fails because the environment is
-            # misconfigured, we silently go with the same writer because this
-            # is too common to happen.  In that case, mojibake is better than
-            # exceptions.
-            binary_writer = _find_binary_writer(text_writer)
-            if binary_writer is None:
-                return text_writer
-
-        # At this point, we default the errors to replace instead of strict
-        # because nobody handles those errors anyways and at this point
-        # we're so fundamentally fucked that nothing can repair it.
+        # Default errors to replace instead of strict in order to get
+        # something that works.
         if errors is None:
-            errors = 'replace'
-        return _make_text_stream(binary_writer, encoding, errors,
-                                 force_writable=force_writable)
+            errors = "replace"
+
+        # Wrap the binary stream in a text stream with the correct
+        # encoding parameters.
+        return _make_text_stream(
+            binary_reader,
+            encoding,
+            errors,
+            force_readable=force_readable,
+            force_writable=force_writable,
+        )
+
+    def _force_correct_text_reader(text_reader, encoding, errors, force_readable=False):
+        return _force_correct_text_stream(
+            text_reader,
+            encoding,
+            errors,
+            _is_binary_reader,
+            _find_binary_reader,
+            force_readable=force_readable,
+        )
+
+    def _force_correct_text_writer(text_writer, encoding, errors, force_writable=False):
+        return _force_correct_text_stream(
+            text_writer,
+            encoding,
+            errors,
+            _is_binary_writer,
+            _find_binary_writer,
+            force_writable=force_writable,
+        )
 
     def get_binary_stdin():
         reader = _find_binary_reader(sys.stdin)
@@ -468,24 +476,45 @@ def get_streerror(e, default=None):
     return msg
 
 
-def open_stream(filename, mode='r', encoding=None, errors='strict',
-                atomic=False):
+def _wrap_io_open(file, mode, encoding, errors):
+    """On Python 2, :func:`io.open` returns a text file wrapper that
+    requires passing ``unicode`` to ``write``. Need to open the file in
+    binary mode then wrap it in a subclass that can write ``str`` and
+    ``unicode``.
+
+    Also handles not passing ``encoding`` and ``errors`` in binary mode.
+    """
+    binary = "b" in mode
+
+    if binary:
+        kwargs = {}
+    else:
+        kwargs = {"encoding": encoding, "errors": errors}
+
+    if not PY2 or binary:
+        return io.open(file, mode, **kwargs)
+
+    f = io.open(file, "b" + mode.replace("t", ""))
+    return _make_text_stream(f, **kwargs)
+
+
+def open_stream(filename, mode='r', encoding=None, errors='strict', atomic=False):
+    binary = "b" in mode
+
     # Standard streams first.  These are simple because they don't need
     # special handling for the atomic flag.  It's entirely ignored.
     if filename == '-':
         if any(m in mode for m in ['w', 'a', 'x']):
-            if 'b' in mode:
+            if binary:
                 return get_binary_stdout(), False
             return get_text_stdout(encoding=encoding, errors=errors), False
-        if 'b' in mode:
+        if binary:
             return get_binary_stdin(), False
         return get_text_stdin(encoding=encoding, errors=errors), False
 
     # Non-atomic writes directly go out through the regular open functions.
     if not atomic:
-        if encoding is None:
-            return open(filename, mode), True
-        return io.open(filename, mode, encoding=encoding, errors=errors), True
+        return _wrap_io_open(filename, mode, encoding, errors), True
 
     # Some usability stuff for atomic writes
     if 'a' in mode:
@@ -513,7 +542,8 @@ def open_stream(filename, mode='r', encoding=None, errors='strict',
         perm = None
 
     flags = os.O_RDWR | os.O_CREAT | os.O_EXCL
-    if 'b' in mode:
+
+    if binary:
         flags |= getattr(os, 'O_BINARY', 0)
 
     while True:
@@ -528,8 +558,8 @@ def open_stream(filename, mode='r', encoding=None, errors='strict',
             if e.errno == errno.EEXIST or (
                 os.name == "nt"
                 and e.errno == errno.EACCES
-                and os.path.isdir(dir)
-                and os.access(dir, os.W_OK)
+                and os.path.isdir(e.filename)
+                and os.access(e.filename, os.W_OK)
             ):
                 continue
             raise
@@ -537,11 +567,7 @@ def open_stream(filename, mode='r', encoding=None, errors='strict',
     if perm is not None:
         os.chmod(tmp_filename, perm)  # in case perm includes bits in umask
 
-    if encoding is not None:
-        f = io.open(fd, mode, encoding=encoding, errors=errors)
-    else:
-        f = os.fdopen(fd, mode)
-
+    f = _wrap_io_open(fd, mode, encoding, errors)
     return _AtomicFile(f, tmp_filename, os.path.realpath(filename)), True
 
 
diff --git a/click/exceptions.py b/click/exceptions.py
index 21db907..6fa1765 100644
--- a/click/exceptions.py
+++ b/click/exceptions.py
@@ -152,19 +152,6 @@ class MissingParameter(BadParameter):
             msg or '',
         )
 
-    def __str__(self):
-        if self.message is None:
-            param_name = self.param.name if self.param else None
-            return "missing parameter: {}".format(param_name)
-        else:
-            return self.message
-
-    if PY2:
-        __unicode__ = __str__
-
-        def __str__(self):
-            return self.__unicode__().encode("utf-8")
-
 
 class NoSuchOption(UsageError):
     """Raised if click attempted to handle an option that does not
diff --git a/docs/commands.rst b/docs/commands.rst
index c32e917..520e418 100644
--- a/docs/commands.rst
+++ b/docs/commands.rst
@@ -88,7 +88,7 @@ script like this:
     @click.pass_context
     def cli(ctx, debug):
         # ensure that ctx.obj exists and is a dict (in case `cli()` is called
-        # by means other than the `if` block below)
+        # by means other than the `if` block below
         ctx.ensure_object(dict)
 
         ctx.obj['DEBUG'] = debug


Test patch:
diff --git a/tests/test_arguments.py b/tests/test_arguments.py
index 4428042..ca48fed 100644
--- a/tests/test_arguments.py
+++ b/tests/test_arguments.py
@@ -203,15 +203,6 @@ def test_missing_arg(runner):
     assert 'Missing argument "ARG".' in result.output
 
 
-def test_missing_argument_string_cast():
-    ctx = click.Context(click.Command(""))
-
-    with pytest.raises(click.MissingParameter) as excinfo:
-        click.Argument(["a"], required=True).full_process_value(ctx, None)
-
-    assert str(excinfo.value) == "missing parameter: a"
-
-
 def test_implicit_non_required(runner):
     @click.command()
     @click.argument('f', default='test')
diff --git a/tests/test_options.py b/tests/test_options.py
index 4ad7f3c..e0f7878 100644
--- a/tests/test_options.py
+++ b/tests/test_options.py
@@ -321,15 +321,6 @@ def test_legacy_options(runner):
     assert result.output == '23\n'
 
 
-def test_missing_option_string_cast():
-    ctx = click.Context(click.Command(""))
-
-    with pytest.raises(click.MissingParameter) as excinfo:
-        click.Option(['-a'], required=True).full_process_value(ctx, None)
-
-    assert str(excinfo.value) == "missing parameter: a"
-
-
 def test_missing_choice(runner):
     @click.command()
     @click.option('--foo', type=click.Choice(['foo', 'bar']),
diff --git a/tests/test_utils.py b/tests/test_utils.py
index 751c6ae..93f1ca0 100644
--- a/tests/test_utils.py
+++ b/tests/test_utils.py
@@ -279,17 +279,18 @@ def test_echo_writing_to_standard_error(capfd, monkeypatch):
 
 
 def test_open_file(runner):
+    @click.command()
+    @click.argument('filename')
+    def cli(filename):
+        with click.open_file(filename) as f:
+            click.echo(f.read())
+
+        click.echo('meep')
+
     with runner.isolated_filesystem():
         with open('hello.txt', 'w') as f:
             f.write('Cool stuff')
 
-        @click.command()
-        @click.argument('filename')
-        def cli(filename):
-            with click.open_file(filename) as f:
-                click.echo(f.read())
-            click.echo('meep')
-
         result = runner.invoke(cli, ['hello.txt'])
         assert result.exception is None
         assert result.output == 'Cool stuff\nmeep\n'
@@ -299,6 +300,44 @@ def test_open_file(runner):
         assert result.output == 'foobar\nmeep\n'
 
 
+def test_open_file_ignore_errors_stdin(runner):
+    @click.command()
+    @click.argument("filename")
+    def cli(filename):
+        with click.open_file(filename, errors="ignore") as f:
+            click.echo(f.read())
+
+    result = runner.invoke(cli, ["-"], input=os.urandom(16))
+    assert result.exception is None
+
+
+def test_open_file_respects_ignore(runner):
+    with runner.isolated_filesystem():
+        with open("test.txt", "w") as f:
+            f.write("Hello world!")
+
+        with click.open_file("test.txt", encoding="utf8", errors="ignore") as f:
+            assert f.errors == "ignore"
+
+
+def test_open_file_ignore_invalid_utf8(runner):
+    with runner.isolated_filesystem():
+        with open("test.txt", "wb") as f:
+            f.write(b"\xe2\x28\xa1")
+
+        with click.open_file("test.txt", encoding="utf8", errors="ignore") as f:
+            f.read()
+
+
+def test_open_file_ignore_no_encoding(runner):
+    with runner.isolated_filesystem():
+        with open("test.bin", "wb") as f:
+            f.write(os.urandom(16))
+
+        with click.open_file("test.bin", errors="ignore") as f:
+            f.read()
+
+
 @pytest.mark.skipif(WIN, reason='os.chmod() is not fully supported on Windows.')
 @pytest.mark.parametrize('permissions', [
     0o400,
@@ -341,21 +380,19 @@ def test_open_file_atomic_permissions_new_file(runner):
         assert stat.S_IMODE(os.stat('new.txt').st_mode) == permissions
 
 
-@pytest.mark.xfail(WIN and not PY2, reason='God knows ...')
 def test_iter_keepopenfile(tmpdir):
     expected = list(map(str, range(10)))
     p = tmpdir.mkdir('testdir').join('testfile')
-    p.write(os.linesep.join(expected))
+    p.write("\n".join(expected))
     with p.open() as f:
         for e_line, a_line in zip(expected, click.utils.KeepOpenFile(f)):
             assert e_line == a_line.strip()
 
 
-@pytest.mark.xfail(WIN and not PY2, reason='God knows ...')
 def test_iter_lazyfile(tmpdir):
     expected = list(map(str, range(10)))
     p = tmpdir.mkdir('testdir').join('testfile')
-    p.write(os.linesep.join(expected))
+    p.write("\n".join(expected))
     with p.open() as f:
         with click.utils.LazyFile(f.name) as lf:
             for e_line, a_line in zip(expected, lf):