From 21c95fc62be492d19c1f7590b027dbc57a418e18 Mon Sep 17 00:00:00 2001 From: pederhan Date: Sun, 6 Oct 2024 11:33:40 +0200 Subject: [PATCH 1/3] Add error msg builder with underlined suggestions --- mreg_cli/errorbuilder.py | 116 ++++++++++++++++++++++++++++++++++++++ mreg_cli/outputmanager.py | 27 +++++---- 2 files changed, 131 insertions(+), 12 deletions(-) create mode 100644 mreg_cli/errorbuilder.py diff --git a/mreg_cli/errorbuilder.py b/mreg_cli/errorbuilder.py new file mode 100644 index 00000000..062e7548 --- /dev/null +++ b/mreg_cli/errorbuilder.py @@ -0,0 +1,116 @@ +"""Builders for error messages with underlined suggestions.""" + +from __future__ import annotations + +import logging +import shlex +from abc import ABC, abstractmethod + +logger = logging.getLogger(__name__) + + +class ErrorBuilder(ABC): + """Base class for building error messages with an underline and suggestion.""" + + # Subclasses should implement this: + SUGGESTION = "" + + # Characters used for formatting the error message + CHAR_UNDERLINE = "^" + CHAR_OFFSET = " " + CHAR_SUGGESTION = "└" + + def __init__(self, command: str, exception: Exception | None = None) -> None: + """Instantiate the builder with the command that caused the error.""" + self.command = command + self.exception = exception + + def get_underline(self, start: int, end: int) -> str: + """Get the underline part of the message.""" + return self.offset(self.CHAR_UNDERLINE * (end - start), start) + + def get_suggestion(self, start: int) -> str: + """Get the suggestion part of the message.""" + msg = self.SUGGESTION + if self.CHAR_SUGGESTION: + msg = f"{self.CHAR_SUGGESTION} {msg}" + return self.offset(msg, start) + + def offset(self, s: str, n: int) -> str: + """Offset the string by n spaces.""" + return " " * n + s + + def build(self) -> str: + """Build the error message with an underline and suggestion.""" + start, end = self.get_offset() + lines = [ + self.command, + self.get_underline(start, end), + self.get_suggestion(start), + ] + return "\n".join(lines) + + @abstractmethod + def get_offset(self) -> tuple[int, int]: + """Compute the start and end index of the underlined part of the command.""" + pass + + +class FilterErrorBuilder(ErrorBuilder): + """Error builder for commands with a filter.""" + + SUGGESTION = "Consider enclosing this part in quotes." + + @staticmethod + def find_word_with_char_offset(command: str, char: str) -> tuple[int, int]: + """Find the start and end index of a word containing a specific character. + + Cannot fail; will always return a tuple of two integers. + """ + command = command.strip() + if not command: + return -1, -1 + + # Parse the command into parts + cmd = shlex.split(command, posix=False) # keep quotes and backslashes + for part in cmd: + # Skip parts that contain quotes + # NOTE: This is a very naive check, and will not work for all cases + # (e.g. escaped quotes) + if "'" in part or '"' in part: + continue + + if char in part: + try: + start, end = command.index(part), command.index(part) + len(part) + if start > end or start >= len(command) or end >= len(command): + raise ValueError(f"Invalid start and end values: {start}, {end}") + except ValueError as e: + logger.error( + "Failed to get index of char '%s' in part '%s' of command %s: %s", + char, + part, + command, + e, + ) + else: + return start, end + return -1, -1 + + def get_offset(self) -> tuple[int, int]: # noqa: D102 (missing docstring [inherit it from parent]) + return self.find_word_with_char_offset(self.command, "|") + + +def get_builder(command: str, exception: Exception | None = None) -> ErrorBuilder | None: + """Get the appropriate error builder for the given command.""" + if "|" in command: + return FilterErrorBuilder(command, exception) + return None + + +def build_error_message(command: str, exception: Exception | None = None) -> str: + """Build an error message with an underline and suggestion.""" + builder = get_builder(command, exception) + if builder: + return builder.build() + return "" diff --git a/mreg_cli/outputmanager.py b/mreg_cli/outputmanager.py index 38e5c5af..b929d834 100644 --- a/mreg_cli/outputmanager.py +++ b/mreg_cli/outputmanager.py @@ -20,20 +20,23 @@ import requests from pydantic import BaseModel -from mreg_cli.exceptions import FileError, InputFailure +from mreg_cli.errorbuilder import build_error_message +from mreg_cli.exceptions import CliError, FileError from mreg_cli.types import JsonMapping, RecordingEntry, TimeInfo logger = logging.getLogger(__name__) @overload -def find_char_outside_quotes(line: str, target_char: str, return_position: Literal[True]) -> int: - ... +def find_char_outside_quotes( + line: str, target_char: str, return_position: Literal[True] +) -> int: ... @overload -def find_char_outside_quotes(line: str, target_char: str, return_position: Literal[False]) -> str: - ... +def find_char_outside_quotes( + line: str, target_char: str, return_position: Literal[False] +) -> str: ... def find_char_outside_quotes( @@ -394,14 +397,14 @@ def get_filter(self, command: str) -> tuple[str, Any, bool]: try: filter_re = re.compile(filter_str) except re.error as exc: - if "|" in command_issued: - raise InputFailure( - "Command parts that contain a pipe ('|') must be quoted.", - ) from exc + base_msg = f"Unable to compile regex '{filter_str}'" + msg = build_error_message(command_issued) + if msg: + # Custom error messages with suggestions go on their own line + raise CliError(f"{base_msg}\n{msg}") from exc else: - raise InputFailure( - f"Unable to compile regex '{filter_str}': {exc}" - ) from exc + # Otherwise append to the base message + raise CliError(f"{base_msg}: {exc}") from exc return (command, filter_re, negate) From a62ad561a684dfac634892ad17172ebf2a768c14 Mon Sep 17 00:00:00 2001 From: pederhan Date: Mon, 7 Oct 2024 09:34:32 +0200 Subject: [PATCH 2/3] Refactor error message building --- mreg_cli/errorbuilder.py | 66 ++++++++++++++++++++++++++++++++------- mreg_cli/outputmanager.py | 9 ++---- 2 files changed, 56 insertions(+), 19 deletions(-) diff --git a/mreg_cli/errorbuilder.py b/mreg_cli/errorbuilder.py index 062e7548..b800f857 100644 --- a/mreg_cli/errorbuilder.py +++ b/mreg_cli/errorbuilder.py @@ -8,6 +8,14 @@ logger = logging.getLogger(__name__) +ExcOrStr = Exception | str + + +# Re-usable function for error builders that don't need a specific implementation +def default_get_offset(self: "ErrorBuilder") -> tuple[int, int]: # noqa: ARG001 + """Get the start and end index of the underlined part of the command.""" + return -1, -1 + class ErrorBuilder(ABC): """Base class for building error messages with an underline and suggestion.""" @@ -20,10 +28,10 @@ class ErrorBuilder(ABC): CHAR_OFFSET = " " CHAR_SUGGESTION = "└" - def __init__(self, command: str, exception: Exception | None = None) -> None: + def __init__(self, command: str, exc_or_str: ExcOrStr) -> None: """Instantiate the builder with the command that caused the error.""" self.command = command - self.exception = exception + self.exc_or_str = exc_or_str def get_underline(self, start: int, end: int) -> str: """Get the underline part of the message.""" @@ -48,12 +56,37 @@ def build(self) -> str: self.get_underline(start, end), self.get_suggestion(start), ] + # prepend the exception message if it exists + if self.exc_or_str: + lines.insert(0, str(self.exc_or_str)) return "\n".join(lines) @abstractmethod def get_offset(self) -> tuple[int, int]: """Compute the start and end index of the underlined part of the command.""" - pass + raise NotImplementedError + + @abstractmethod + def can_build(self) -> bool: + """Check if the builder can build an error message for the given command.""" + raise NotImplementedError + + +class FallbackErrorBuilder(ErrorBuilder): + """Builder for errors without a specific implementation. + + Renders the error message as a single line without an underline or suggestion. + The error message is displayed as-is. + """ + + get_offset = default_get_offset # needed for ABC parent class + + def build(self) -> str: + """Build the error message without an underline or suggestion.""" + return str(self.exc_or_str) + + def can_build(self) -> bool: # noqa: D102 + return True class FilterErrorBuilder(ErrorBuilder): @@ -100,17 +133,26 @@ def find_word_with_char_offset(command: str, char: str) -> tuple[int, int]: def get_offset(self) -> tuple[int, int]: # noqa: D102 (missing docstring [inherit it from parent]) return self.find_word_with_char_offset(self.command, "|") + def can_build(self) -> bool: + """Check if the builder can build an error message for the given command.""" + return "|" in self.command + + +BUILDERS: list[type[ErrorBuilder]] = [ + FilterErrorBuilder, +] + -def get_builder(command: str, exception: Exception | None = None) -> ErrorBuilder | None: +def get_builder(command: str, exc_or_str: ExcOrStr) -> ErrorBuilder: """Get the appropriate error builder for the given command.""" - if "|" in command: - return FilterErrorBuilder(command, exception) - return None + for builder in BUILDERS: + b = builder(command, exc_or_str) + if b.can_build(): + return b + return FallbackErrorBuilder(command, exc_or_str) -def build_error_message(command: str, exception: Exception | None = None) -> str: +def build_error_message(command: str, exc_or_str: ExcOrStr) -> str: """Build an error message with an underline and suggestion.""" - builder = get_builder(command, exception) - if builder: - return builder.build() - return "" + builder = get_builder(command, exc_or_str) + return builder.build() diff --git a/mreg_cli/outputmanager.py b/mreg_cli/outputmanager.py index b929d834..bf40fa67 100644 --- a/mreg_cli/outputmanager.py +++ b/mreg_cli/outputmanager.py @@ -398,13 +398,8 @@ def get_filter(self, command: str) -> tuple[str, Any, bool]: filter_re = re.compile(filter_str) except re.error as exc: base_msg = f"Unable to compile regex '{filter_str}'" - msg = build_error_message(command_issued) - if msg: - # Custom error messages with suggestions go on their own line - raise CliError(f"{base_msg}\n{msg}") from exc - else: - # Otherwise append to the base message - raise CliError(f"{base_msg}: {exc}") from exc + msg = build_error_message(command_issued, base_msg) + raise CliError(msg) from exc return (command, filter_re, negate) From ae8e1b800bb904ede60c7fbb6cf1da654ab14d30 Mon Sep 17 00:00:00 2001 From: pederhan Date: Mon, 7 Oct 2024 09:40:55 +0200 Subject: [PATCH 3/3] Add docstrings --- mreg_cli/errorbuilder.py | 51 +++++++++++++++++++++++++++++++++------- 1 file changed, 43 insertions(+), 8 deletions(-) diff --git a/mreg_cli/errorbuilder.py b/mreg_cli/errorbuilder.py index b800f857..4b55a30e 100644 --- a/mreg_cli/errorbuilder.py +++ b/mreg_cli/errorbuilder.py @@ -34,22 +34,40 @@ def __init__(self, command: str, exc_or_str: ExcOrStr) -> None: self.exc_or_str = exc_or_str def get_underline(self, start: int, end: int) -> str: - """Get the underline part of the message.""" + """Get the underline part of the message. + + :param int start: The start index of the underlined part of the command. + :param int end: The end index of the underlined part of the command. + + :returns: The underlined part of the command. + """ return self.offset(self.CHAR_UNDERLINE * (end - start), start) def get_suggestion(self, start: int) -> str: - """Get the suggestion part of the message.""" + """Get the suggestion part of the message. + + :param int start: The start index of the underlined part of the command. + """ msg = self.SUGGESTION if self.CHAR_SUGGESTION: msg = f"{self.CHAR_SUGGESTION} {msg}" return self.offset(msg, start) def offset(self, s: str, n: int) -> str: - """Offset the string by n spaces.""" + """Offset the string by n spaces. + + :param str s: The string to offset. + :param int n: The number of spaces to offset by. + + :returns: The offset string. + """ return " " * n + s def build(self) -> str: - """Build the error message with an underline and suggestion.""" + """Build the error message with an underline and suggestion. + + :returns: The error message with an underline and suggestion. + """ start, end = self.get_offset() lines = [ self.command, @@ -99,6 +117,12 @@ def find_word_with_char_offset(command: str, char: str) -> tuple[int, int]: """Find the start and end index of a word containing a specific character. Cannot fail; will always return a tuple of two integers. + + :param str command: The command to search. + :param str char: The character to search for. + + :returns: Tuple of two integers representing the start and end index of the word. + Returns (-1, -1) if the character is not found. """ command = command.strip() if not command: @@ -133,8 +157,7 @@ def find_word_with_char_offset(command: str, char: str) -> tuple[int, int]: def get_offset(self) -> tuple[int, int]: # noqa: D102 (missing docstring [inherit it from parent]) return self.find_word_with_char_offset(self.command, "|") - def can_build(self) -> bool: - """Check if the builder can build an error message for the given command.""" + def can_build(self) -> bool: # noqa: D102 (missing docstring [inherit it from parent]) return "|" in self.command @@ -144,7 +167,13 @@ def can_build(self) -> bool: def get_builder(command: str, exc_or_str: ExcOrStr) -> ErrorBuilder: - """Get the appropriate error builder for the given command.""" + """Get the appropriate error builder for the given command. + + :param str command: The command that caused the error. + :param exc_or_str: The exception or error message. + + :returns: An error builder instance. + """ for builder in BUILDERS: b = builder(command, exc_or_str) if b.can_build(): @@ -153,6 +182,12 @@ def get_builder(command: str, exc_or_str: ExcOrStr) -> ErrorBuilder: def build_error_message(command: str, exc_or_str: ExcOrStr) -> str: - """Build an error message with an underline and suggestion.""" + """Build an error message for the given command and exception. + + :param str command: The command that caused the error. + :param exc_or_str: The exception or error message. + + :returns: Error message based on the command and exception. + """ builder = get_builder(command, exc_or_str) return builder.build()