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" %} {% include "common/components/field.html" with field=form.source label='File' classes="js-agree-with-terms-trigger js-submit-form-file-input" %}
</div> </div>
</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="row">
<div class="col mx-4 mt-4"> <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" %} {% 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': { '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': { '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': { 'invalid-no-manifest.zip': {
'source': ['The manifest file is missing.'], 'source': ['The manifest file is missing.'],
}, },
'invalid-manifest-toml.zip': {'source': ['Could not parse the manifest file.']}, 'invalid-manifest-toml.zip': {'source': ['Manifest file contains invalid code.']},
'invalid-theme-multiple-xmls.zip': {'source': ['A theme should have exactly one XML file.']}, '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': { 'invalid-missing-wheels.zip': {
'source': [ '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.' 'The manifest file should be at the top level of the archive, or one level deep.'
), ),
# TODO: surface TOML parsing errors? # TODO: surface TOML parsing errors?
'invalid_manifest_toml': _('Could not parse the manifest file.'), 'invalid_manifest_toml': _('Manifest file contains invalid code.'),
'invalid_missing_init': _('An add-on should have an __init__.py file.'), 'invalid_missing_init': mark_safe(_('Add-on file missing: <strong>__init__.py</strong>.')),
'missing_or_multiple_theme_xml': _('A theme should have exactly one XML file.'), 'missing_or_multiple_theme_xml': mark_safe(_('Themes can only contain <strong>one XML file</strong>.')),
'invalid_zip_archive': msg_only_zip_files, 'invalid_zip_archive': msg_only_zip_files,
'missing_manifest_toml': _('The manifest file is missing.'), 'missing_manifest_toml': _('The manifest file is missing.'),
'missing_wheel': _('A declared wheel is missing in the zip file, expected path: %(path)s'), 'missing_wheel': _('Python wheel missing: %(path)s'), # TODO <strong>%(path)s</strong>
'forbidden_filepaths': _('Archive contains forbidden files or directories: %(paths)s'), 'forbidden_filepaths': _('Archive contains forbidden files or directories: %(paths)s'), #TODO <strong>%(paths)s</strong>
} }
class Meta: class Meta:

View File

@ -172,10 +172,21 @@ def find_forbidden_filepaths(file_list):
def validate_file_list(toml_content, manifest_filepath, file_list): def validate_file_list(toml_content, manifest_filepath, file_list):
"""Check the files in in the archive against manifest.""" """Check the files in in the archive against manifest."""
error_codes = [] 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'] type_slug = toml_content['type']
if type_slug == 'theme': if type_slug == 'theme':
theme_xmls = filter_paths_by_ext(file_list, '.xml') 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') error_codes.append('missing_or_multiple_theme_xml')
elif type_slug == 'add-on': elif type_slug == 'add-on':
# __init__.py is expected to be next to the manifest # __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( error_codes.append(
{'code': 'missing_wheel', 'params': {'path': expected_wheel_path}} {'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 return error_codes