Improve validation messages #196

Merged
Dalai Felinto merged 7 commits from pr-error-messages into main 2024-07-01 15:22:55 +02:00
5 changed files with 30 additions and 19 deletions

View File

@ -59,6 +59,13 @@
{% include "common/components/field.html" with field=form.source label='File' classes="js-agree-with-terms-trigger js-submit-form-file-input" %}
</div>
</div>
{% if form.errors %}
<blockquote class="mb-0"><i class="i-info"></i>
Build your extension using the <a class="text-underline" href="https://docs.blender.org/manual/en/dev/advanced/extensions/command_line_arguments.html#subcommand-build">command-line tool</a> to catch and prevent most of these errors.
</blockquote>
{% endif %}
<div class="row">
<div class="col mx-4 mt-4">
{% include "common/components/field.html" with field=form.agreed_with_terms classes="js-agree-with-terms-trigger js-agree-with-terms-checkbox" %}

View File

@ -109,19 +109,20 @@ EXPECTED_VALIDATION_ERRORS = {
],
},
'invalid-addon-no-init.zip': {
'source': ['An add-on should have an __init__.py file.'],
'source': ['Add-on file missing: <strong>__init__.py</strong>.'],
},
'invalid-addon-dir-no-init.zip': {
'source': ['An add-on should have an __init__.py file.'],
'source': ['Add-on file missing: <strong>__init__.py</strong>.'],
},
'invalid-no-manifest.zip': {
'source': ['The manifest file is missing.'],
},
'invalid-manifest-toml.zip': {'source': ['Could not parse the manifest file.']},
'invalid-theme-multiple-xmls.zip': {'source': ['A theme should have exactly one XML file.']},
'invalid-manifest-toml.zip': {'source': ['Manifest file contains invalid code.']},
'invalid-theme-multiple-xmls.zip': {'source': ['Themes can only contain <strong>one XML file</strong>.']},
'invalid-theme-multiple-xmls-macosx.zip': {'source': ['Archive contains forbidden files or directories: __MACOSX/']},
'invalid-missing-wheels.zip': {
'source': [
'A declared wheel is missing in the zip file, expected path: addon/wheels/test-wheel-whatever.whl'
'Python wheel missing: addon/wheels/test-wheel-whatever.whl'
]
},
}

View File

@ -34,13 +34,13 @@ class FileForm(forms.ModelForm):
'The manifest file should be at the top level of the archive, or one level deep.'
),
# TODO: surface TOML parsing errors?
'invalid_manifest_toml': _('Could not parse the manifest file.'),
'invalid_missing_init': _('An add-on should have an __init__.py file.'),
'missing_or_multiple_theme_xml': _('A theme should have exactly one XML file.'),
'invalid_manifest_toml': _('Manifest file contains invalid code.'),
'invalid_missing_init': mark_safe(_('Add-on file missing: <strong>__init__.py</strong>.')),
'missing_or_multiple_theme_xml': mark_safe(_('Themes can only contain <strong>one XML file</strong>.')),
'invalid_zip_archive': msg_only_zip_files,
'missing_manifest_toml': _('The manifest file is missing.'),
'missing_wheel': _('A declared wheel is missing in the zip file, expected path: %(path)s'),
'forbidden_filepaths': _('Archive contains forbidden files or directories: %(paths)s'),
'missing_wheel': _('Python wheel missing: %(path)s'), # TODO <strong>%(path)s</strong>
'forbidden_filepaths': _('Archive contains forbidden files or directories: %(paths)s'), #TODO <strong>%(paths)s</strong>
}
class Meta:

View File

@ -172,10 +172,21 @@ def find_forbidden_filepaths(file_list):
def validate_file_list(toml_content, manifest_filepath, file_list):
"""Check the files in in the archive against manifest."""
error_codes = []
found_forbidden_filepaths = find_forbidden_filepaths(file_list)
if found_forbidden_filepaths:
error_codes.append(
{
dfelinto marked this conversation as resolved Outdated

let's restructure this check a bit to prevent possible confusion in the future:

  • now the whole theme branch is skipped, while we mean to skip only a particular check
  • if in the future more theme-related checks are added, we would likely want them to run

i.e. let's move the and '__MACOSX/' not in found_forbidden_filepaths part to the if len(list(theme_xmls)) != 1 line

let's restructure this check a bit to prevent possible confusion in the future: - now the whole theme branch is skipped, while we mean to skip only a particular check - if in the future more theme-related checks are added, we would likely want them to run i.e. let's move the `and '__MACOSX/' not in found_forbidden_filepaths` part to the `if len(list(theme_xmls)) != 1` line

and it's better to move the found_forbidden_filepaths logic closer to the variable declaration - i.e. let's move the whole block up, not just the variable declaration

and it's better to move the `found_forbidden_filepaths` logic closer to the variable declaration - i.e. let's move the whole block up, not just the variable declaration
'code': 'forbidden_filepaths',
'params': {'paths': ', '.join(found_forbidden_filepaths)},
}
)
type_slug = toml_content['type']
if type_slug == 'theme':
theme_xmls = filter_paths_by_ext(file_list, '.xml')
if len(list(theme_xmls)) != 1:
# Special treatment for Mac, so the same problem (__MACOSX folders)
# doesn't lead to two errors showing.
if len(list(theme_xmls)) != 1 and '__MACOSX/' not in found_forbidden_filepaths:
error_codes.append('missing_or_multiple_theme_xml')
elif type_slug == 'add-on':
# __init__.py is expected to be next to the manifest
@ -192,14 +203,6 @@ def validate_file_list(toml_content, manifest_filepath, file_list):
error_codes.append(
{'code': 'missing_wheel', 'params': {'path': expected_wheel_path}}
)
found_forbidden_filepaths = find_forbidden_filepaths(file_list)
if found_forbidden_filepaths:
error_codes.append(
{
'code': 'forbidden_filepaths',
'params': {'paths': ', '.join(found_forbidden_filepaths)},
}
)
return error_codes