From 44e73682dd4f5312a6f3608de36cd6e83a2e02ed Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Fri, 26 Jan 2024 12:06:57 +0100 Subject: [PATCH] Notifications: improve copy on error messages (#11062) * Notifications: de-duplicate/standardize concurrency limit reached * Notifications: improve copy * Remove unused `message_id` * Improve missing/not found `mkdocs.yml` file Closes #7707 * Improve `sphinx.configuration` file not found error message This validation should eventually happen at the Read the Docs config file. We are not there, but at least we are giving a better error message to users. Closes #8352 * Improve copy for error messages * Pass `format_values` to format the filename properly in the message * Apply suggestions from code review Co-authored-by: Anthony * Show the type of the received value * Better header * Lint * Render class name via custom template filter ``` In [4]: n.get_message().get_rendered_body() Out[4]: '\n\n\nConfig validation error in .\nExpected a list, got type int (422).' ``` * Make the title clearer * Remove "\n\n\n" due to tests failing --------- Co-authored-by: Anthony --- readthedocs/config/exceptions.py | 1 - readthedocs/config/notifications.py | 55 ++++----- readthedocs/doc_builder/backends/mkdocs.py | 7 +- readthedocs/doc_builder/backends/sphinx.py | 5 +- readthedocs/doc_builder/exceptions.py | 1 - .../doc_builder/python_environments.py | 14 ++- readthedocs/notifications/messages.py | 106 +++++++++--------- .../templatetags/notifications_filters.py | 9 ++ readthedocs/projects/tasks/builds.py | 5 +- 9 files changed, 106 insertions(+), 97 deletions(-) create mode 100644 readthedocs/notifications/templatetags/notifications_filters.py diff --git a/readthedocs/config/exceptions.py b/readthedocs/config/exceptions.py index ea76ce0cbcf..637ffd71cf7 100644 --- a/readthedocs/config/exceptions.py +++ b/readthedocs/config/exceptions.py @@ -11,7 +11,6 @@ class ConfigError(BuildUserError): "config:python:use-system-site-packages-removed" ) INVALID_VERSION = "config:base:invalid-version" - GENERIC_INVALID_CONFIG_KEY = "config:key:generic-invalid-config-key" NOT_BUILD_TOOLS_OR_COMMANDS = "config:build:missing-build-tools-commands" BUILD_JOBS_AND_COMMANDS = "config:build:jobs-and-commands" APT_INVALID_PACKAGE_NAME_PREFIX = "config:apt:invalid-package-name-prefix" diff --git a/readthedocs/config/notifications.py b/readthedocs/config/notifications.py index f073dd1c6a2..b6f744c6af0 100644 --- a/readthedocs/config/notifications.py +++ b/readthedocs/config/notifications.py @@ -29,7 +29,8 @@ body=_( textwrap.dedent( """ - No default configuration file found at repository's root. + The required readthedocs.yaml configuration file was not found at repository's root. + Learn how to use this file in our configuration file tutorial. """ ).strip(), ), @@ -66,8 +67,8 @@ textwrap.dedent( """ The configuration key python.system_packages has been deprecated and removed. - Refer to https://blog.readthedocs.com/drop-support-system-packages/ to read more - about this change and how to upgrade your config file." + Read our blog post + to learn more about this change and how to upgrade your configuration file." """ ).strip(), ), @@ -80,8 +81,8 @@ textwrap.dedent( """ The configuration key python.use_system_site_packages has been deprecated and removed. - Refer to https://blog.readthedocs.com/drop-support-system-packages/ to read more - about this change and how to upgrade your config file." + Read our blog post + to learn more about this change and how to upgrade your configuration file." """ ).strip(), ), @@ -99,29 +100,13 @@ ), type=ERROR, ), - Message( - id=ConfigError.GENERIC_INVALID_CONFIG_KEY, - header=_("Invalid configuration option"), - body=_( - textwrap.dedent( - """ - Invalid configuration option: {{key}}. - - Read the Docs configuration file: {{source_file}}. - - {{error_message}} - """ - ).strip(), - ), - type=ERROR, - ), Message( id=ConfigError.NOT_BUILD_TOOLS_OR_COMMANDS, - header=_("Invalid configuration option: build"), + header=_("Missing configuration option"), body=_( textwrap.dedent( """ - At least one item should be provided in "tools" or "commands". + At least one of the following configuration options is required: build.tools or build.commands. """ ).strip(), ), @@ -169,7 +154,8 @@ body=_( textwrap.dedent( """ - You need to install your project with pip to use extra_requirements. + You need to install your project with python.install.method: pip + to use python.install.extra_requirements. """ ).strip(), ), @@ -177,11 +163,12 @@ ), Message( id=ConfigError.PIP_PATH_OR_REQUIREMENT_REQUIRED, - header=_("Invalid configuration key"), + header=_("Missing configuration key"), body=_( textwrap.dedent( """ - path or requirements key is required for python.install. + When using python.install, + one of the following keys are required: python.install.path or python.install.requirements. """ ).strip(), ), @@ -205,7 +192,7 @@ body=_( textwrap.dedent( """ - You can not have exclude and include submodules at the same time. + You can not have submodules.exclude and submodules.include at the same time. """ ).strip(), ), @@ -230,6 +217,7 @@ textwrap.dedent( """ Error while parsing {{filename}}. + Make sure your configuration file doesn't have any errors. {{error_message}} """ @@ -261,7 +249,8 @@ textwrap.dedent( """ Config validation error in {{key}}. - Expected one of (0, 1, true, false), got {{value}}. + Expected one of [0, 1, true, false], got type {{value|to_class_name}} ({{value}}). + Make sure the type of the value is not a string. """ ).strip(), ), @@ -274,7 +263,9 @@ textwrap.dedent( """ Config validation error in {{key}}. - Expected one of ({{choices}}), got {{value}}. + Expected one of ({{choices}}), got type {{value|to_class_name}} ({{value}}). + Double check the type of the value. + A string may be required (e.g. "3.10" insted of 3.10) """ ).strip(), ), @@ -287,7 +278,7 @@ textwrap.dedent( """ Config validation error in {{key}}. - Expected a dictionary, got {{value}}. + Expected a dictionary, got type {{value|to_class_name}} ({{value}}). """ ).strip(), ), @@ -326,7 +317,7 @@ textwrap.dedent( """ Config validation error in {{key}}. - Expected a string, got {{value}}. + Expected a string, got type {{value|to_class_name}} ({{value}}). """ ).strip(), ), @@ -339,7 +330,7 @@ textwrap.dedent( """ Config validation error in {{key}}. - Expected a list, got {{value}}. + Expected a list, got type {{value|to_class_name}} ({{value}}). """ ).strip(), ), diff --git a/readthedocs/doc_builder/backends/mkdocs.py b/readthedocs/doc_builder/backends/mkdocs.py index 570b1a93404..cd4a256e0b1 100644 --- a/readthedocs/doc_builder/backends/mkdocs.py +++ b/readthedocs/doc_builder/backends/mkdocs.py @@ -168,9 +168,10 @@ def append_conf(self): value = [] if not isinstance(value, list): raise MkDocsYAMLParseError( - MkDocsYAMLParseError.INVALID_EXTRA_CONFIG.format( - config=config, - ), + message_id=MkDocsYAMLParseError.INVALID_EXTRA_CONFIG, + format_values={ + "extra_config": config, + }, ) # Add the static file only if isn't already in the list. value.extend([extra for extra in extras if extra not in value]) diff --git a/readthedocs/doc_builder/backends/sphinx.py b/readthedocs/doc_builder/backends/sphinx.py index b4d2ddc6988..7997e018b2f 100644 --- a/readthedocs/doc_builder/backends/sphinx.py +++ b/readthedocs/doc_builder/backends/sphinx.py @@ -258,7 +258,10 @@ def append_conf(self): if not os.path.exists(self.config_file): raise UserFileNotFound( - UserFileNotFound.FILE_NOT_FOUND.format(self.config_file) + message_id=UserFileNotFound.FILE_NOT_FOUND, + format_values={ + "filename": os.path.relpath(self.config_file, self.project_path), + }, ) # Allow symlinks, but only the ones that resolve inside the base directory. diff --git a/readthedocs/doc_builder/exceptions.py b/readthedocs/doc_builder/exceptions.py index b13e9ba2b74..0e67422a6b4 100644 --- a/readthedocs/doc_builder/exceptions.py +++ b/readthedocs/doc_builder/exceptions.py @@ -35,7 +35,6 @@ class BuildAppError(BuildBaseException): class BuildUserError(BuildBaseException): GENERIC = "build:user:generic" SKIPPED_EXIT_CODE_183 = "build:user:exit-code-183" - MAX_CONCURRENCY = "build:user:max-concurrency" BUILD_COMMANDS_WITHOUT_OUTPUT = "build:user:output:no-html" BUILD_OUTPUT_IS_NOT_A_DIRECTORY = "build:user:output:is-no-a-directory" diff --git a/readthedocs/doc_builder/python_environments.py b/readthedocs/doc_builder/python_environments.py index c11c0f10553..4af234e64b3 100644 --- a/readthedocs/doc_builder/python_environments.py +++ b/readthedocs/doc_builder/python_environments.py @@ -291,9 +291,10 @@ def _append_core_requirements(self): ) if not inputfile: raise UserFileNotFound( - UserFileNotFound.FILE_NOT_FOUND.format( - self.config.conda.environment - ) + message_id=UserFileNotFound.FILE_NOT_FOUND, + format_values={ + "filename": self.config.conda.environment, + }, ) environment = parse_yaml(inputfile) except IOError: @@ -334,9 +335,10 @@ def _append_core_requirements(self): ) if not outputfile: raise UserFileNotFound( - UserFileNotFound.FILE_NOT_FOUND.format( - self.config.conda.environment - ) + message_id=UserFileNotFound.FILE_NOT_FOUND, + format_values={ + "filename": self.config.conda.environment, + }, ) yaml.safe_dump(environment, outputfile) except IOError: diff --git a/readthedocs/notifications/messages.py b/readthedocs/notifications/messages.py index 4684f566ae1..28c43b453ff 100644 --- a/readthedocs/notifications/messages.py +++ b/readthedocs/notifications/messages.py @@ -58,18 +58,24 @@ def get_display_icon_classes(self): return " ".join(classes) + def _prepend_template_prefix(self, template): + """ + Prepend Django {% load %} template tag. + + This is required to render the notifications with custom filters/tags. + """ + prefix = "{% load notifications_filters %}" + return prefix + template + def get_rendered_header(self): - template = Template(self.header) + template = Template(self._prepend_template_prefix(self.header)) return template.render(context=Context(self.format_values)) def get_rendered_body(self): - template = Template(self.body) + template = Template(self._prepend_template_prefix(self.body)) return template.render(context=Context(self.format_values)) -# TODO: review the copy of these notifications/messages on PR review and adapt them. -# Most of them are copied from what we had in `readthedocs.doc_builder.exceptions` -# and slightly adapted to have a header and a better body. BUILD_MESSAGES = [ Message( id=BuildAppError.GENERIC_WITH_BUILD_ID, @@ -122,11 +128,12 @@ def get_rendered_body(self): body=_( textwrap.dedent( """ - Concurrency limit reached ({{limit}}), retrying in 5 minutes. + Your project, organization, or user has reached its maximum number of concurrent builds allowed ({{limit}}). + This build will automatically retry in 5 minutes. """ ).strip(), ), - type=INFO, + type=ERROR, ), Message( id=BuildCancelled.CANCELLED_BY_USER, @@ -142,7 +149,7 @@ def get_rendered_body(self): ), Message( id=BuildUserError.SKIPPED_EXIT_CODE_183, - header=_("Build skipped manually."), + header=_("Build skipped."), body=_( textwrap.dedent( """ @@ -155,11 +162,12 @@ def get_rendered_body(self): ), Message( id=BuildUserError.BUILD_TIME_OUT, - header=_("Build exited due to time out."), + header=_("Build terminated due to time out."), body=_( textwrap.dedent( """ - Build exited due to time out. + The build was terminated due to time out. + Read more about time and memory limits in our documentation. """ ).strip(), ), @@ -167,11 +175,12 @@ def get_rendered_body(self): ), Message( id=BuildUserError.BUILD_EXCESSIVE_MEMORY, - header=_("Build exited due to excessive memory consumption."), + header=_("Build terminated due to excessive memory consumption."), body=_( textwrap.dedent( """ - Build exited due to excessive memory consumption. + This build was terminated due to excessive memory consumption. + Read more about time and memory limits in our documentation. """ ).strip(), ), @@ -179,11 +188,11 @@ def get_rendered_body(self): ), Message( id=BuildAppError.BUILD_DOCKER_UNKNOWN_ERROR, - header=_("Build exited due to unknown error."), + header=_("Build terminated due to unknown error."), body=_( textwrap.dedent( """ - Build exited due to unknown error: {{message}} + This build was terminated due to unknown error: {{message}} """ ).strip(), ), @@ -203,19 +212,6 @@ def get_rendered_body(self): ), type=ERROR, ), - Message( - id=BuildUserError.MAX_CONCURRENCY, - header=_("Concurrency limit reached"), - body=_( - textwrap.dedent( - """ - Your project, organization, or user is currently building the maximum concurrency builds allowed ({{limit}}). - It will automatically retry in 5 minutes. - """ - ).strip(), - ), - type=WARNING, - ), Message( id=BuildUserError.BUILD_COMMANDS_WITHOUT_OUTPUT, header=_("No HTML content found"), @@ -235,6 +231,7 @@ def get_rendered_body(self): textwrap.dedent( """ Build output directory for format "{{artifact_type}}" is not a directory. + Make sure you created this directory properly when running build.commands. """ ).strip(), ), @@ -273,7 +270,7 @@ def get_rendered_body(self): body=_( textwrap.dedent( """ - Your documentation did not generate an 'index.html' at its root directory. + Your documentation did not generate an index.html at its root directory. This is required for documentation serving at the root URL for this version. """ ).strip(), @@ -286,9 +283,9 @@ def get_rendered_body(self): body=_( textwrap.dedent( """ - Some files were detected in an unsupported output path, '_build/html'. + Some files were detected in an unsupported output path: _build/html. Ensure your project is configured to use the output path - '$READTHEDOCS_OUTPUT/html' instead. + $READTHEDOCS_OUTPUT/html instead. """ ).strip(), ), @@ -302,7 +299,7 @@ def get_rendered_body(self): """ The configuration file required to build documentation is missing from your project. Add a configuration file to your project to make it build successfully. - Read more at https://docs.readthedocs.io/en/stable/config-file/v2.html + Read more in our configuration file documentation. """ ).strip(), ), @@ -314,9 +311,9 @@ def get_rendered_body(self): body=_( textwrap.dedent( """ - The configuration key "build.image" is deprecated. - Use "build.os" instead to continue building your project. - Read more at https://docs.readthedocs.io/en/stable/config-file/v2.html#build-os + The configuration key build.image is deprecated. + Use build.os instead to continue building your project. + Read more in our configuration file documentation. """ ).strip(), ), @@ -328,13 +325,14 @@ def get_rendered_body(self): body=_( textwrap.dedent( """ - The configuration key "build.os" is required to build your documentation. - Read more at https://docs.readthedocs.io/en/stable/config-file/v2.html#build-os + The configuration key build.os is required to build your documentation. + Read more in our configuration file documentation. """ ).strip(), ), type=ERROR, ), + # TODO: consider exposing the name of the file exceeding the size limit. Message( id=BuildUserError.FILE_TOO_LARGE, header=_("There is at least one file that exceeds the size limit"), @@ -355,6 +353,7 @@ def get_rendered_body(self): textwrap.dedent( f""" PDF file was not generated/found in "{BUILD_COMMANDS_OUTPUT_PATH_HTML}/pdf" output directory. + Make sure the PDF file is saved in this directory. """ ).strip(), ), @@ -366,7 +365,7 @@ def get_rendered_body(self): body=_( textwrap.dedent( """ - The "build.commands" feature is in beta, and could have backwards incompatible changes while in beta. + The build.commands feature is in beta, and could have backwards incompatible changes while in beta. Read more at our documentation to find out its limitations and potential issues. """ ).strip(), @@ -379,7 +378,7 @@ def get_rendered_body(self): body=_( textwrap.dedent( """ - No TeX files were found. + Read the Docs could not generate a PDF file because the intermediate step generating the TeX file failed. """ ).strip(), ), @@ -402,11 +401,11 @@ def get_rendered_body(self): ), Message( id=MkDocsYAMLParseError.INVALID_DOCS_DIR_CONFIG, - header=_(""), + header=_("MkDocs docs_dir configuration option is invalid"), body=_( textwrap.dedent( """ - The "docs_dir" config from your MkDocs YAML config file has to be a + The docs_dir option from your mkdocs.yml configuration file has to be a string with relative or absolute path. """ ).strip(), @@ -415,10 +414,12 @@ def get_rendered_body(self): ), Message( id=MkDocsYAMLParseError.INVALID_DOCS_DIR_PATH, - header=_(""), + header=_("MkDocs docs_dir path not found"), body=_( textwrap.dedent( """ + The path specified by docs_dir in the mkdocs.yml file does not exist. + Make sure this path is correct. """ ).strip(), ), @@ -426,11 +427,13 @@ def get_rendered_body(self): ), Message( id=MkDocsYAMLParseError.INVALID_EXTRA_CONFIG, - header=_(""), + header=_( + "MkDocs {{extra_config}} configuration option is invalid" + ), body=_( textwrap.dedent( """ - The "{{config}}" config from your MkDocs YAML config file has to be a + The {{extra_config}} option from your mkdocs.yml configuration file has to be a list of relative paths. """ ).strip(), @@ -439,11 +442,11 @@ def get_rendered_body(self): ), Message( id=MkDocsYAMLParseError.EMPTY_CONFIG, - header=_(""), + header=_("MkDocs configuration file is empty"), body=_( textwrap.dedent( """ - Please make sure the MkDocs YAML configuration file is not empty. + Please make sure the mkdocs.yml configuration file is not empty. """ ).strip(), ), @@ -451,12 +454,13 @@ def get_rendered_body(self): ), Message( id=MkDocsYAMLParseError.NOT_FOUND, - header=_(""), + header=_("MkDocs configuration file not found"), body=_( textwrap.dedent( """ - A configuration file was not found. - Make sure you have a "mkdocs.yml" file in your repository. + The configuration file for MkDocs was not found. + Make sure the mkdocs.configuration option is correct, + and you have the mkdocs.yml in that location. """ ).strip(), ), @@ -464,12 +468,12 @@ def get_rendered_body(self): ), Message( id=MkDocsYAMLParseError.CONFIG_NOT_DICT, - header=_(""), + header=_("Unknown error when loading your MkDocs configuration file"), body=_( textwrap.dedent( """ - Your MkDocs YAML config file is incorrect. - Please follow the user guide https://www.mkdocs.org/user-guide/configuration/ + Your mkdocs.yml configuration file is incorrect. + Please follow the official user guide to configure the file properly. """ ).strip(), diff --git a/readthedocs/notifications/templatetags/notifications_filters.py b/readthedocs/notifications/templatetags/notifications_filters.py new file mode 100644 index 00000000000..96ad0101fc3 --- /dev/null +++ b/readthedocs/notifications/templatetags/notifications_filters.py @@ -0,0 +1,9 @@ +from django import template + +register = template.Library() + + +@register.filter +def to_class_name(value): + """Output the name of the class for the given object.""" + return value.__class__.__name__ diff --git a/readthedocs/projects/tasks/builds.py b/readthedocs/projects/tasks/builds.py index ed7c15b668c..69570d2994a 100644 --- a/readthedocs/projects/tasks/builds.py +++ b/readthedocs/projects/tasks/builds.py @@ -295,18 +295,19 @@ class UpdateDocsTask(SyncRepositoryMixin, Task): RepositoryError, MkDocsYAMLParseError, ProjectConfigurationError, + BuildMaxConcurrencyError, ) # Do not send notifications on failure builds for these exceptions. exceptions_without_notifications = ( BuildCancelled.CANCELLED_BY_USER, - BuildUserError.MAX_CONCURRENCY, BuildUserError.SKIPPED_EXIT_CODE_183, BuildAppError.BUILDS_DISABLED, + BuildMaxConcurrencyError.LIMIT_REACHED, ) # Do not send external build status on failure builds for these exceptions. - exceptions_without_external_build_status = (BuildUserError.MAX_CONCURRENCY,) + exceptions_without_external_build_status = (BuildMaxConcurrencyError.LIMIT_REACHED,) acks_late = True track_started = True