Allowed setters and getters to take additional arguments (superseded by #78991)

[nlupugla]

Closes godotengine/godot-proposals#6750.
Closes godotengine/godot-proposals#3096.
Closes godotengine/godot-proposals#6107 (discussion).
Closes #74144.

addresses the following proposals:
godotengine/godot-proposals#2817.
godotengine/godot-proposals#2983.
godotengine/godot-proposals#4867.
godotengine/godot-proposals#4868.

For example, the following is now possible:

@tool
extends Node2D

@onready var sprite := $Icon as Sprite2D
@export var offset : Vector2 : set=set_sprite_property, get=get_sprite_property
@export var centered : bool : set=set_sprite_property, get=get_sprite_property


func get_sprite_property(property : StringName) -> Variant:
	return sprite.get(property)
	
	
func set_sprite_property(property : StringName, value : Variant) -> void:
	sprite.set(property, value)

Previously, one would have had to use a more verbose and tedious approach:

@tool
extends Node2D

@onready var sprite := $Icon as Sprite2D
@export var offset : Vector2 : set=set_sprite_offset, get=get_sprite_offset
@export var centered : bool : set=set_sprite_centered, get=get_sprite_centered


func get_sprite_offset() -> Variant:
	return sprite.get("offset")
	
	
func set_sprite_offset(value : Variant) -> void:
	sprite.set("offset", value)


func get_sprite_centered() -> Variant:
	return sprite.get("centered")
	
	
func set_sprite_property(value : Variant) -> void:
	sprite.set("centered", value)

TODO:

• Attach some GDScript tests.
• Require that the first argument of a 2-argument setter and a 1-argument getter be a StringName

[nlupugla]

I noticed the tests complained about "name" shadowing a class variable. I wrote a few lines like Variant name = p_name;. I changed them to Variant property_name = p_name, so hopefully that fixes the issue.

[dalexeev]

I tested and reviewed this PR and added some changes: signature analysis, support for optional arguments (godotengine/godot-proposals#6107, vnen's comment), tests.

Commit: dalexeev@19b170a
Patch: review.patch.txt

You can apply the patch with git apply or git am (and then you need to squash the commits into one). You can credit me as a co-author if you wish.

[nlupugla]

Thanks for the review @dalexeev :)
I applied your patch and pushed the changes, but I don't think I quite figured out the whole rebasing thing. I tried to squash my commits together and credit you as a coauthor, but got confused somewhere along the way...

[dalexeev]

There should be one commit. Use rebase, don't merge master into your branch.

[dalexeev]

This is a bug. This is why I prefer not to use --gdscript-generate-tests but to edit .out manually, even if it's tedious.

[dalexeev]

This file does not follow the GDScript style guide (wrong spaces, symbolic logical operators, missing trailing newline). This is undesirable in non-parser tests. In addition, I do not see the point in this test, in my opinion it duplicates the ones I have already added.

[nlupugla]

Ah, I was going to ask you about this test on Rocket. I had written this a week or two ago before your patch. I noticed it was redundant with your tests. I can remove it.

[nlupugla]

Thanks for preparing the squashed commit! Unless there's a rush to get the PR accepted, I want to see if I can figure out how to do it on my own, just for self-education. If there is a rush, let me know and I'll go ahead and apply the patch :)

[nlupugla]

Alright, I think I managed to figure out how to squash the commits and credit you. Thanks for the help :)
Just so you know, I haven't had a chance yet to test the changes you made. Should I trust that your changes are good and remove my "draft" status or would you appreciate if I did some testing?

[dalexeev]

Alright, I think I managed to figure out how to squash the commits and credit you.

No, it doesn't look correct. You just rolled back to your old version and added my tests, but the rest of the changes were discarded (signature analysis, support for optional arguments, etc.). Also, Co-authored-by is incorrect.

Should I trust that your changes are good and remove my "draft" status or would you appreciate if I did some testing?

I ran the tests locally, on my PC they complete successfully. Also I tested manually, looks correct.

Sorry for making the review in the form of an "ultimatum patch", I respect your desire to figure it out on your own. I just want to help you and save time, yours and mine. In any case, I want to say that as a first contribution, your work is already impressive!

[nlupugla]

Thanks :)
I think I fixed it now.

[nlupugla]

Nice. I'm recompiling on my machine just to test things out again. When I'm satisfied, I'll remove my draft status.

[dalexeev]

Suggested change

	if (setter_function->mandatory_parameter_count > 2) {
	if (setter_function->mandatory_parameter_count == 0 || setter_function->mandatory_parameter_count > 2) {

[nlupugla]

Fixed it.
Thanks for the help and patience!

[nlupugla]

I did some brief testing on my end and everything seemed to check out. I removed the draft status from my PR.

I edited my original post so that it mentions being able to have optional arguments in the setters and getters. Should I edit the title of my PR as well, or will that mess something up?

[dalexeev]

Looks good to me! There is only a minor suggestion for naming CallError variables. I think this PR is ready for review by the GDScript team.

[nlupugla]

Looks good to me! There is only a minor suggestion for naming CallError variables. I think this PR is ready for review by the GDScript team.

Awesome :)

So should I name the call errors to err, ce, or call_err?

Also, what are the next steps for updating the documentation on setters/getters to reflect the changes? Should I start writing a PR to update the docs now, or wait until these changes are approved?

[dalexeev]

So should I name the call errors to err, ce, or call_err?

It seems to me that the name err is worse, since it is usually used for bool or Error codes. For CallError, the names ce, call_err, or the full call_error can be used. But in any case, it has the lowest priority. Things like renaming local/internal variables, fixing comment style, adding/removing blank lines are best done on a case-by-case basis, on adjacent lines, as long as it doesn't require a lot of effort and doesn't interfere with the perception of major changes. You are not required to bring the entire file to a single style, there is no point or benefit in this.

Should I start writing a PR to update the docs now, or wait until these changes are approved?

I don't think this is strictly regulated, but in practice PRs for docs and/or demo projects are usually opened after the PR in the main repository is merged. Since the docs cannot be updated before the main PR is merged. Plus, during the review process, you may be asked to make some changes, which will also require you to update the docs PR.

Therefore, in my opinion, it is better to wait for the final version, so as not to do extra work. It's normal for the documentation (Manual) to be a bit behind the current state, no one expects it to be updated the same day a feature is in master. The exception is the Class Reference, since it's in the main repository, we try to keep it up to date.

[nlupugla]

Something failed in the Github Actions Workflow run that ran last week. It seemed unrelated to the changes on my branch, so I just updated my branch to be synced with the current upstream/master with the hopes that it will fix the problem. Is there a way for me to run the workflow tests locally so I don't have to wait on Github Actions to be sure the commit is okay?

[dalexeev]

Yes, you can:

scons dev_build=true tests=true
bin/<godot executable> --test

But this is only for your platform. Tests on GitHub run for different platforms and compilation flags.

[nlupugla]

Ah, gotcha. The failure was on Linux and I have Windows, so I guess there's not much I can do but hope it passes this time :)

[nlupugla]

Hi @dalexeev :)

I updated my branch to be concurrent with master. Let me know if there's anything else I need to do to help the PR move along :)

[AThousandShips]

This is a new feature and therefore unlikely to be included in 4.1, as we are in feature freeze, so chances are it will have to wait for approval until after the release of 4.1, to focus on things for 4.1 (Hopefully that won't take too long either!)

[nlupugla]

Reworked the error messages as suggested.

[dalexeev]

Need a rebase after #77129.

[AThousandShips]

You reset the branch to the base branch, which closes the PR automatically

[AThousandShips]

Superseded by: #78991