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

C#: Fix GD0107 not applying to arrays and dictionaries containing nodes #94599

Merged

Conversation

juanjp600
Copy link
Contributor

Fixes #94589.

@juanjp600 juanjp600 requested a review from a team as a code owner July 21, 2024 20:08
@AThousandShips AThousandShips added bug topic:dotnet cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Jul 22, 2024
@AThousandShips AThousandShips added this to the 4.4 milestone Jul 22, 2024
@raulsntos
Copy link
Member

Thanks for contributing to the .NET module! This makes sense to me, and GDScript already reports an error for exported Array[Node] variables.

This PR currently only applies to Node[] but the diagnostic should also be reported for Godot.Collections.Array<Node>.

Also, since you are a new contributor, make sure to read CONTRIBUTING.md and the contributing documentation if you haven't already.

Feel free to reach out in the development chat if you need help.

@juanjp600 juanjp600 force-pushed the node-array-export-diagnostic branch from 54c5cdb to c5980ba Compare July 22, 2024 19:54
@juanjp600 juanjp600 changed the title dotnet: Fix GD0107 not applying to arrays of nodes dotnet: Fix GD0107 not applying to arrays and dictionaries containing nodes Jul 22, 2024
@juanjp600
Copy link
Contributor Author

Nice catch, thanks! I've added checks for Godot.Collection.Array<T> and Godot.Collections.Dictionary<TKey, TValue>.

Here's a new resource class to test with:

using Godot;
using System;

[GlobalClass]
public partial class AResource : Resource
{
	[Export] public ANode3D[] DotnetArrayField;
	[Export] public Godot.Collections.Array<ANode3D> GodotArrayField;
	[Export] public Godot.Collections.Dictionary<ANode3D, string> GodotDictionaryWithNodeAsKeyField;
	[Export] public Godot.Collections.Dictionary<string, ANode3D> GodotDictionaryWithNodeAsValueField;
	
	[Export] public ANode3D[] DotnetArrayProperty { get; set; }
	[Export] public Godot.Collections.Array<ANode3D> GodotArrayProperty { get; set; }
	[Export] public Godot.Collections.Dictionary<ANode3D, string> GodotDictionaryWithNodeAsKeyProperty { get; set; }
	[Export] public Godot.Collections.Dictionary<string, ANode3D> GodotDictionaryWithNodeAsValueProperty { get; set; }
}

@juanjp600 juanjp600 force-pushed the node-array-export-diagnostic branch from c5980ba to d386429 Compare July 22, 2024 19:58
@raulsntos
Copy link
Member

You'll also need to add tests for the new cases that are expected to report the diagnostic in the ExportDiagnostics_GD0107.cs file.

The C# you just shared is pretty close, just need to add {|GD0107:Identifier|} in the places were the GD0107 diagnostic is expected.

@juanjp600 juanjp600 force-pushed the node-array-export-diagnostic branch from d386429 to 8812ed6 Compare July 23, 2024 14:54
@juanjp600 juanjp600 force-pushed the node-array-export-diagnostic branch from 8812ed6 to 206ff9e Compare July 23, 2024 18:06
@juanjp600 juanjp600 force-pushed the node-array-export-diagnostic branch from 206ff9e to 8aa444d Compare July 24, 2024 05:20
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks!

@akien-mga akien-mga changed the title dotnet: Fix GD0107 not applying to arrays and dictionaries containing nodes C#: Fix GD0107 not applying to arrays and dictionaries containing nodes Aug 16, 2024
@akien-mga akien-mga merged commit 0d0eb71 into godotengine:master Aug 16, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@juanjp600 juanjp600 deleted the node-array-export-diagnostic branch August 17, 2024 01:14
@akien-mga
Copy link
Member

Cherry-picked for 4.3.1.

@akien-mga akien-mga removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Sep 16, 2024
@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Godot.NET.Sdk's GD0107 does not apply to exported members of type Node[]
4 participants