infrastructure/extensions-website
4
8
Fork 10
Code Issues 20 Pull Requests 2 Projects Releases Wiki Activity

Improve validation messages #196

Merged
Dalai Felinto merged 7 commits from pr-error-messages into main 2024-07-01 15:22:55 +02:00
Conversation 8 Commits 7 Files Changed 5 +16 -7
3 changed files with 16 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 0a273a7fc7 - Show all commits

7
extensions/templates/extensions/submit.html
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 %}
<div>
To catch and prevent most of these errors, it is recommended to 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>.
</div>
{% 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" %}

10
files/forms.py
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:

6
files/utils.py
View File

@ -173,7 +173,10 @@ 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 = []
type_slug = toml_content['type'] type_slug = toml_content['type']
if type_slug == 'theme': found_forbidden_filepaths = find_forbidden_filepaths(file_list)
# Special treatment for Mac, so the same problem (__MACOSX folders) doesn't lead to two errors showing.
if type_slug == 'theme' and '__MACOSX/' not in found_forbidden_filepaths:
dfelinto marked this conversation as resolved Outdated
Oleg-Komarov commented 2024-07-01 15:11:03 +02:00
Outdated
Review
Copy Link

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
Oleg-Komarov commented 2024-07-01 15:14:17 +02:00
Outdated
Review
Copy Link

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
theme_xmls = filter_paths_by_ext(file_list, '.xml') theme_xmls = filter_paths_by_ext(file_list, '.xml')
if len(list(theme_xmls)) != 1: if len(list(theme_xmls)) != 1:
error_codes.append('missing_or_multiple_theme_xml') error_codes.append('missing_or_multiple_theme_xml')
@ -192,7 +195,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: if found_forbidden_filepaths:
error_codes.append( error_codes.append(
{ {