Improve validation messages #196

Dalai Felinto · 2024-06-27T17:10:10+02:00

Dalai Felinto commented

2024-06-27 17:10:10 +02:00

Prevent duplicate XML error to show up if the __MACOSX error is already present.
Add a general "Use the command-line tools" message on the template if any error is present.
Use <strong> to highlight some elements of the error messages.

This is marginally related to: #175.

TODO: The errors which depend on parameters cannot use and be marked safe.

This is to be tackled as a separate patch. For now they were removed from this patch.

The screengrab includes them already, but only for future reference.

1. Prevent duplicate XML error to show up if the __MACOSX error is already present. 2. Add a general "Use the command-line tools" message on the template if any error is present. 3. Use `<strong>` to highlight some elements of the error messages. This is marginally related to: #175. --- ![image](/attachments/b0b14301-dc81-489e-95a2-4839ad44b502) --- TODO: The errors which depend on parameters cannot use <strong> and be marked safe. This is to be tackled as a separate patch. For now they were removed from this patch. The screengrab includes them already, but only for future reference.

image.png

114 KiB

image.png

117 KiB

Dalai Felinto commented

2024-06-27 17:18:12 +02:00

My test file: __MACOSX_missing_wheel.zip

My test file: [__MACOSX_missing_wheel.zip](/attachments/593e3d4e-84ef-4e45-bb00-594beb5df077)

__MACOSX_missing_wheel.zip

7.5 KiB

Dalai Felinto requested review from Oleg-Komarov 2024-06-27 17:21:19 +02:00

Pablo Vazquez commented

2024-06-27 17:27:34 +02:00

How about moving the gist of the message to the beginning of the phrase?

From:

To catch and prevent most of these errors, it is recommended to build your extension using the command-line tool.

to:

Build your extension using the command-line tool to catch and prevent most of these errors.

How about moving the gist of the message to the beginning of the phrase? From: > To catch and prevent most of these errors, it is recommended to build your extension using the **command-line tool**. to: > Build your extension using the **command-line tool** to catch and prevent most of these errors.

Dalai Felinto commented

2024-06-27 17:32:26 +02:00

How about moving the gist of the message to the beginning of the phrase?

From:

To catch and prevent most of these errors, it is recommended to build your extension using the command-line tool.

to:

Build your extension using the command-line tool to catch and prevent most of these errors.

+1, updated patch

> How about moving the gist of the message to the beginning of the phrase? > > From: > > To catch and prevent most of these errors, it is recommended to build your extension using the **command-line tool**. > > to: > > > Build your extension using the **command-line tool** to catch and prevent most of these errors. +1, updated patch

Oleg-Komarov commented

2024-06-30 15:47:02 +02:00

Prevent duplicate XML error to show up if the __MACOSX error is already present.

I am not sure this particular bit is a good change:

a __MACOSX may be present and not contain any .xml files, in this case we still want to check for multiple xml files, but this change would prevent that
potentially, a better change might be to include the names of the multiple xml files in the error message, so it is clear what we are complaining about
an even better step, in my opinion, is to standardize the expected path for the theme xml in the archive, then "multiple xml" check won't be needed at all, UPD: a theme archive must contain only blender_manifest.toml and theme.xml, that's it.

This is marginally related to: #175

We need to clearly communicate in that ticket why the error is happening and what it means:

we will not accept archives that contain files not intended for end-users (i.e. __MACOSX and friends)
the fact that macos finder creates zip archives that are not suitable for distribution via Extensions Platform is not a bug of the Extensions Platform - a different tool should be used (the instructions being added in this PR should hopefully be sufficient to inform the users)

> Prevent duplicate XML error to show up if the __MACOSX error is already present. I am not sure this particular bit is a good change: - a __MACOSX may be present and not contain any .xml files, in this case we still want to check for multiple xml files, but this change would prevent that - potentially, a better change might be to include the names of the multiple xml files in the error message, so it is clear what we are complaining about - an even better step, in my opinion, is to standardize the expected path for the theme xml in the archive, then "multiple xml" check won't be needed at all, UPD: a theme archive **must** contain only `blender_manifest.toml` and `theme.xml`, that's it. > This is marginally related to: #175 We need to clearly communicate in that ticket why the error is happening and what it means: - we will not accept archives that contain files not intended for end-users (i.e. __MACOSX and friends) - the fact that macos finder creates zip archives that are not suitable for distribution via Extensions Platform is not a bug of the Extensions Platform - a different tool should be used (the instructions being added in this PR should hopefully be sufficient to inform the users)

Dalai Felinto commented

2024-07-01 11:31:06 +02:00

a __MACOSX may be present and not contain any .xml files, in this case we still want to check for multiple xml files, but this change would prevent that

I'm not overly concerned about this. This is such an unlikely corner case (two XMLs + __MACOSX) that I don't mind the person uploading the theme a few times until getting this right.

Specially because the only way to reliably by-pass the ___MACOSX error is by using the build command. Which in turn will already check for all the errors.

> a __MACOSX may be present and not contain any .xml files, in this case we still want to check for multiple xml files, but this change would prevent that I'm not overly concerned about this. This is such an unlikely corner case (two XMLs + __MACOSX) that I don't mind the person uploading the theme a few times until getting this right. Specially because the only way to reliably by-pass the ___MACOSX error is by using the build command. Which in turn will already check for all the errors.

Oleg-Komarov commented

2024-07-01 11:41:20 +02:00

Specially because the only way to reliably by-pass the ___MACOSX error is by using the build command. Which in turn will already check for all the errors.

I think that it is not the only way:
using zip from command line will also work zip theme.zip blender_manifest.toml theme.xml

Tests need to be updated:

error message changes break 5 test cases
we should add a new test case that demonstrates the behavior of suppressing one error when the other is present

> Specially because the only way to reliably by-pass the ___MACOSX error is by using the build command. Which in turn will already check for all the errors. I think that it is not the only way: using zip from command line will also work `zip theme.zip blender_manifest.toml theme.xml` Tests need to be updated: - error message changes break 5 test cases - we should add a new test case that demonstrates the behavior of suppressing one error when the other is present

Dalai Felinto referenced this pull request from blender/blender

2024-07-01 11:54:19 +02:00

Extensions: Build option should validate tags as well #123986

Dalai Felinto force-pushed pr-error-messages from cc65bdd4e9 to 3a811a5002

2024-07-01 14:43:09 +02:00

Compare

Dalai Felinto added 1 commit 2024-07-01 14:54:38 +02:00

From review: new unittest for OSX bd4f430758

Dalai Felinto commented

2024-07-01 15:04:02 +02:00

@Oleg-Komarov I updated the patch. I think we can leave the refactoring of the parsing logic as a separate patch.

Which means this patch could get in as it is without changing the error message for missing_wheel and forbidden_filepaths.

@Oleg-Komarov I updated the patch. I think we can leave the refactoring of the parsing logic as a separate patch. Which means this patch could get in as it is without changing the error message for missing_wheel and forbidden_filepaths.

Dalai Felinto changed title from ~~WIP: Improve validation messages~~ to Improve validation messages

2024-07-01 15:08:08 +02:00

Oleg-Komarov reviewed 2024-07-01 15:11:03 +02:00

files/utils.py Outdated

						
				@ -177,0 +176,4 @@

				    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:

Oleg-Komarov commented

2024-07-01 15:11:03 +02:00

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

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

dfelinto marked this conversation as resolved

Dalai Felinto added 1 commit 2024-07-01 15:19:42 +02:00

From review: Move logic around 4fe07c2204

Dalai Felinto commented

2024-07-01 15:19:49 +02:00

@Oleg-Komarov done

Oleg-Komarov approved these changes 2024-07-01 15:21:18 +02:00

Oleg-Komarov left a comment

🚢

Dalai Felinto merged commit 8e33d18b43 into main

2024-07-01 15:22:55 +02:00

Dalai Felinto deleted branch pr-error-messages