Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve validation, call _get_configuration_warnings() automatically on property Node changed #9759

Open
m21-cerutti opened this issue May 16, 2024 · 1 comment

Comments

@m21-cerutti
Copy link

m21-cerutti commented May 16, 2024

Describe the project you are working on

A experimental game to learn.

Describe the problem or limitation you are having in your project

I wanted to implement an elegant way to warn me if an exported property on my node is not set, or is invalid whatever the property.

I have seen about _get_configuration_warnings() documentation and tried to implement it, but I think it's kind of really boilerplate and counter-intuitive to call update_configuration_warnings() in setter, and by a magic of warning variable, or surcharge the _get_configuration_warning(), to get effectively the warnings called.

I would like a more automatic way, since warnings is an error prevention system really useful, that should not be painful to implement.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

What I propose would be either :

  • _get_configuration_warnings being called automatically when an exported property changed
  • Add a signal changed like resources and property without getter/setter emit a it by default
  • Add a signal changed and add an option inside @export to emit changed signal, where by default it not emit it.

For those 2 last solutions above we can than connect the signal explicitly to update_configuration_warnings()

  • add a @validate(function_name) to validate a property with a defined function returning a PackedStringArray that would be added to _get_configuration_warnings() result (by default [ ]), called when property changed even with setter.

I would prefer this solution since it could cumulate easily with getter/setter, _get_configuration_warnings for more complex case and be more clear of the usage.

Note that it would be complementary to #3004, and the mechanism should be useful also to non tool script.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Initial script from 4.2 example:

@tool

@export var title = "":
	set(p_title):
		if p_title != title:
			title = p_title
			update_configuration_warnings()

@export var description = "":
	set(p_description):
		if p_description != description:
			description = p_description
			update_configuration_warnings()


func _get_configuration_warnings():
	var warnings = []

	if title == "":
		warnings.append("Please set `title` to a non-empty value.")

	if description.length() >= 100:
		warnings.append("`description` should be less than 100 characters long.")

	# Returning an empty array means "no warning".
	return warnings

Signal changed emitted, with option variant to not set it by default:

@tool

@export("changed") var title = ""
@export("changed") var description = ""

func _get_configuration_warnings():
	var warnings = []

	if title == "":
		warnings.append("Please set `title` to a non-empty value.")

	if description.length() >= 100:
		warnings.append("`description` should be less than 100 characters long.")

	# Returning an empty array means "no warning".
	return warnings

func _ready() -> void:
	changed.connect(update_configuration_warnings)

Like you see with the signal approach, it remove the set/get syntax, that is unnecessarily verbose, and avoid to forget update_configuration_warnings(), that would be the lightest breaking change for this improvement.

And my favourite, validate annotation:

@tool

@validate("_not_empty_validate")
@export  var title = ""

@validate("_length_validate")
@export var description = ""

func _not_empty_validate(property_name, value):
	if value == "":
		return ["Please set `%s` to a non-empty value." % property_name)]

func _length_validate(property_name, value):
	if value == "":
		return [ "`%s` should be less than 100 characters long." % property_name ]

It's more readable about what we are validating, reusable (even more if we can put an array of function for validate annotation), and really compact and straightforward. We completely hiding the _get_configuration_warnings() syntax if it's not needed and we don't have to write getter/setter.

If this enhancement will not be used often, can it be worked around with a few lines of script?

It's an improvement to not write dozen line of code to just systematically call update_configuration_warnings.

Is there a reason why this should be core and not an add-on in the asset library?

It's a tool script improvement and is closely binded to the editor, I would expect this as core and all people could profit it.

@m21-cerutti m21-cerutti changed the title Call _get_configuration_warning() automatically on Node changed Improve validation, call _get_configuration_warning() automatically on property Node changed May 16, 2024
@m21-cerutti m21-cerutti changed the title Improve validation, call _get_configuration_warning() automatically on property Node changed Improve validation, call _get_configuration_warnings() automatically on property Node changed May 16, 2024
@m21-cerutti
Copy link
Author

m21-cerutti commented May 16, 2024

Would resolve in a more general way #8343.
And could maybe be merged with _validate_property mechanism (since it would change the return type, but value can be retrivieve with the property_name).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants