Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Handling Encoding Error. #1496

Closed
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
157 changes: 84 additions & 73 deletions isort/api.py
Expand Up @@ -13,6 +13,7 @@
FileSkipComment,
FileSkipSetting,
IntroducedSyntaxErrors,
UnsupportedEncodingError,
)
from .format import ask_whether_to_apply_changes_to_file, create_terminal_printer, show_unified_diff
from .io import Empty
Expand Down Expand Up @@ -260,16 +261,20 @@ def check_file(
- **extension**: The file extension that contains imports. Defaults to filename extension or py.
- ****config_kwargs**: Any config modifications.
"""
with io.File.read(filename) as source_file:
return check_stream(
source_file.stream,
show_diff=show_diff,
extension=extension,
config=config,
file_path=file_path or source_file.path,
disregard_skip=disregard_skip,
**config_kwargs,
)
try:
with io.File.read(filename) as source_file:
return check_stream(
source_file.stream,
show_diff=show_diff,
extension=extension,
config=config,
file_path=file_path or source_file.path,
disregard_skip=disregard_skip,
**config_kwargs,
)

except UnsupportedEncodingError:
raise UnsupportedEncodingError(filename)


def sort_file(
Expand Down Expand Up @@ -297,70 +302,76 @@ def sort_file(
- **write_to_stdout**: If `True`, write to stdout instead of the input file.
- ****config_kwargs**: Any config modifications.
"""
with io.File.read(filename) as source_file:
actual_file_path = file_path or source_file.path
config = _config(path=actual_file_path, config=config, **config_kwargs)
changed: bool = False
try:
if write_to_stdout:
changed = sort_stream(
input_stream=source_file.stream,
output_stream=sys.stdout,
config=config,
file_path=actual_file_path,
disregard_skip=disregard_skip,
extension=extension,
)
else:
tmp_file = source_file.path.with_suffix(source_file.path.suffix + ".isorted")
try:
with tmp_file.open(
"w", encoding=source_file.encoding, newline=""
) as output_stream:
shutil.copymode(filename, tmp_file)
changed = sort_stream(
input_stream=source_file.stream,
output_stream=output_stream,
config=config,
file_path=actual_file_path,
disregard_skip=disregard_skip,
extension=extension,
)
if changed:
if show_diff or ask_to_apply:
source_file.stream.seek(0)
with tmp_file.open(
encoding=source_file.encoding, newline=""
) as tmp_out:
show_unified_diff(
file_input=source_file.stream.read(),
file_output=tmp_out.read(),
file_path=actual_file_path,
output=None if show_diff is True else cast(TextIO, show_diff),
color_output=config.color_output,
)
if show_diff or (
ask_to_apply
and not ask_whether_to_apply_changes_to_file(
str(source_file.path)
try:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the try except should be elsewhere (probably on the call of this function, or , better on encoding, _ = tokenize.detect_encoding(buffer.readline) in io.py so it's more localized and the change affect less code than that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Pierre-Sassoulas I have also added a try clause on encoding, _ = tokenize.detect_encoding in isort/io.py. The reason why I also put a try catch block here as well as isort/main.py is that I'm trying to throw a warning for each file with invalid encoding(if verbose flag is set) and not an error even if at least one file has valid encoding. The error is only for the case when all the files processed have invalid encodings. So I needed a way to propagate this invalid encodings exceptions to isort/main.py where I am finally deciding if to throw an error or not. isort/io.py and isort/api.py are only called per file so I couldn't come up with a way to do it there.

with io.File.read(filename) as source_file:
actual_file_path = file_path or source_file.path
config = _config(path=actual_file_path, config=config, **config_kwargs)
changed: bool = False
try:
if write_to_stdout:
changed = sort_stream(
input_stream=source_file.stream,
output_stream=sys.stdout,
config=config,
file_path=actual_file_path,
disregard_skip=disregard_skip,
extension=extension,
)
else:
tmp_file = source_file.path.with_suffix(source_file.path.suffix + ".isorted")
try:
with tmp_file.open(
"w", encoding=source_file.encoding, newline=""
) as output_stream:
shutil.copymode(filename, tmp_file)
changed = sort_stream(
input_stream=source_file.stream,
output_stream=output_stream,
config=config,
file_path=actual_file_path,
disregard_skip=disregard_skip,
extension=extension,
)
if changed:
if show_diff or ask_to_apply:
source_file.stream.seek(0)
with tmp_file.open(
encoding=source_file.encoding, newline=""
) as tmp_out:
show_unified_diff(
file_input=source_file.stream.read(),
file_output=tmp_out.read(),
file_path=actual_file_path,
output=None
if show_diff is True
else cast(TextIO, show_diff),
color_output=config.color_output,
)
):
return False
source_file.stream.close()
tmp_file.replace(source_file.path)
if not config.quiet:
print(f"Fixing {source_file.path}")
finally:
try: # Python 3.8+: use `missing_ok=True` instead of try except.
tmp_file.unlink()
except FileNotFoundError:
pass
except ExistingSyntaxErrors:
warn(f"{actual_file_path} unable to sort due to existing syntax errors")
except IntroducedSyntaxErrors: # pragma: no cover
warn(f"{actual_file_path} unable to sort as isort introduces new syntax errors")

return changed
if show_diff or (
ask_to_apply
and not ask_whether_to_apply_changes_to_file(
str(source_file.path)
)
):
return False
source_file.stream.close()
tmp_file.replace(source_file.path)
if not config.quiet:
print(f"Fixing {source_file.path}")
finally:
try: # Python 3.8+: use `missing_ok=True` instead of try except.
tmp_file.unlink()
except FileNotFoundError:
pass
except ExistingSyntaxErrors:
warn(f"{actual_file_path} unable to sort due to existing syntax errors")
except IntroducedSyntaxErrors: # pragma: no cover
warn(f"{actual_file_path} unable to sort as isort introduces new syntax errors")

return changed

except UnsupportedEncodingError:
raise UnsupportedEncodingError(filename)


def _config(
Expand Down
13 changes: 12 additions & 1 deletion isort/exceptions.py
@@ -1,5 +1,6 @@
"""All isort specific exception classes should be defined here"""
from typing import Any, Dict
from pathlib import Path
from typing import Any, Dict, Union

from .profiles import profiles

Expand Down Expand Up @@ -157,3 +158,13 @@ def __init__(self, unsupported_settings: Dict[str, Dict[str, str]]):
"https://pycqa.github.io/isort/docs/configuration/options/.\n"
)
self.unsupported_settings = unsupported_settings


class UnsupportedEncodingError(ISortError):
"""Raised when isort has introduced an encoding error while reading a file"""

def __init__(
self,
filename: Union[str, Path],
):
super().__init__(f"Unknown encoding in {filename}")
4 changes: 3 additions & 1 deletion isort/io.py
Expand Up @@ -6,6 +6,8 @@
from pathlib import Path
from typing import Iterator, NamedTuple, TextIO, Union

from .exceptions import UnsupportedEncodingError

_ENCODING_PATTERN = re.compile(br"^[ \t\f]*#.*?coding[:=][ \t]*([-_.a-zA-Z0-9]+)")


Expand Down Expand Up @@ -37,7 +39,7 @@ def _open(filename):
return text
except Exception:
buffer.close()
raise
raise UnsupportedEncodingError(filename)

@staticmethod
@contextmanager
Expand Down
30 changes: 28 additions & 2 deletions isort/main.py
Expand Up @@ -10,7 +10,7 @@
from warnings import warn

from . import __version__, api, sections
from .exceptions import FileSkipped
from .exceptions import FileSkipped, UnsupportedEncodingError
from .format import create_terminal_printer
from .logo import ASCII_ART
from .profiles import profiles
Expand Down Expand Up @@ -67,9 +67,12 @@


class SortAttempt:
def __init__(self, incorrectly_sorted: bool, skipped: bool) -> None:
def __init__(
self, incorrectly_sorted: bool, skipped: bool, supported_encoding: bool = True
) -> None:
self.incorrectly_sorted = incorrectly_sorted
self.skipped = skipped
self.supported_encoding = supported_encoding


def sort_imports(
Expand Down Expand Up @@ -101,6 +104,11 @@ def sort_imports(
except FileSkipped:
skipped = True
return SortAttempt(incorrectly_sorted, skipped)

except UnsupportedEncodingError:
if config.verbose:
warn(f"Encoding not supported for {file_name}")
return SortAttempt(incorrectly_sorted, skipped, False)
except (OSError, ValueError) as error:
warn(f"Unable to parse file {file_name} due to {error}")
return None
Expand Down Expand Up @@ -842,6 +850,7 @@ def main(argv: Optional[Sequence[str]] = None, stdin: Optional[TextIOWrapper] =
remapped_deprecated_args = config_dict.pop("remapped_deprecated_args", False)
wrong_sorted_files = False
all_attempt_broken = False
no_valid_encodings = False

if "src_paths" in config_dict:
config_dict["src_paths"] = {
Expand Down Expand Up @@ -917,9 +926,18 @@ def main(argv: Optional[Sequence[str]] = None, stdin: Optional[TextIOWrapper] =

# If any files passed in are missing considered as error, should be removed
is_no_attempt = True
unsupported_encodings = 0
total_files_scanned = 0
for sort_attempt in attempt_iterator:
if not sort_attempt:
continue # pragma: no cover - shouldn't happen, satisfies type constraint

total_files_scanned += 1

if not sort_attempt.supported_encoding:
unsupported_encodings += 1
continue

incorrectly_sorted = sort_attempt.incorrectly_sorted
if arguments.get("check", False) and incorrectly_sorted:
wrong_sorted_files = True
Expand All @@ -929,6 +947,9 @@ def main(argv: Optional[Sequence[str]] = None, stdin: Optional[TextIOWrapper] =
)
is_no_attempt = False

if total_files_scanned and unsupported_encodings == total_files_scanned:
no_valid_encodings = True

num_skipped += len(skipped)
if num_skipped and not arguments.get("quiet", False):
if config.verbose:
Expand Down Expand Up @@ -971,6 +992,11 @@ def main(argv: Optional[Sequence[str]] = None, stdin: Optional[TextIOWrapper] =
if all_attempt_broken:
sys.exit(1)

if no_valid_encodings:
printer = create_terminal_printer(color=config.color_output)
printer.error("No valid encodings.")
sys.exit(1)


if __name__ == "__main__":
main()
28 changes: 28 additions & 0 deletions tests/unit/test_main.py
Expand Up @@ -316,6 +316,34 @@ def test_main(capsys, tmpdir):
main.main(["-sp", str(config_file), "-"], stdin=input_content)


def test_unsupported_encodings(tmpdir, capsys):
tmp_file = tmpdir.join("file.py")
tmp_file.write(
"# [syntax-error]\n"
"# -*- coding: IBO-8859-1 -*-\n"
'""" check correct unknown encoding declaration\n'
'"""\n'
"\n"
"__revision__ = 'יייי'\n"
)

# should throw an error if only unsupported encoding provided
with pytest.raises(SystemExit):
main.main([str(tmp_file)])
out, error = capsys.readouterr()

assert "No valid encodings." in error

# should not throw an error if at least one valid encoding found
normal_file = tmpdir.join("file1.py")
normal_file.write("import os\nimport sys")

main.main([str(tmp_file), str(normal_file)])
out, error = capsys.readouterr()

assert not error


def test_isort_command():
"""Ensure ISortCommand got registered, otherwise setuptools error must have occurred"""
assert main.ISortCommand