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

Warn when creating a script with the same name as the parent class #47686

Merged
merged 1 commit into from
Apr 16, 2021

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Apr 7, 2021

This PR adds a warning when creating a script if the class name is the same as the parent class.

Screenshot from 2021-04-06 22-46-01

This is most useful for C# users, but it's also relevant for GDScript/VisualScript/GDNative users as well (for those, it's a best practice thing, not a technical limitation, or rather, a technical inconvenience).

For example, let's say that you created this Enemy class in C#, which is a KinematicBody3D:

public partial class Enemy : KinematicBody3D

Then you want to create a class for your player, but you forgot to rename the node to "Player" or similar, and you also forgot to change the name of the file to something other than KinematicBody3D.cs. So you get this:

public partial class KinematicBody3D : Godot.KinematicBody3D

The C# script generator has code to handle this case, so it will mark it as extending Godot.KinematicBody3D. However, this breaks the Enemy class, since now the behavior and variables from the player will show up inside Enemy, since Enemy is now extending KinematicBody3D from KinematicBody3D.cs instead of Godot.KinematicBody3D. To be clear, this is not a hypothetical problem, I've seen this happen to someone in the past.

  • One solution would be to make all new scripts : Godot.Something instead of : Something, however there would still be confusion if someone used new KinematicBody3D instead of new Godot.KinematicBody3D.

  • Also, even if this problem didn't exist (and it doesn't in GDScript etc), I would still recommend as a best practice that people don't name their scripts the same as a built-in type for the sake of clarity.

The solution I've come up with is this PR which will warn users if they try to create a script with the same name as the parent type. Really we would want to check if the class name is the same as any built-in type, but I'm not sure what's the best way to implement that, and checking if it's the same as the parent class will cover the most common cases for this warning.

@aaronfranke aaronfranke added enhancement topic:editor usability topic:dotnet cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Apr 7, 2021
@aaronfranke aaronfranke added this to the 4.0 milestone Apr 7, 2021
@aaronfranke aaronfranke requested review from Calinou, neikeq and a team April 7, 2021 03:05
@akien-mga akien-mga merged commit 0e72d3d into godotengine:master Apr 16, 2021
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the script-name-warning branch April 16, 2021 19:54
@akien-mga
Copy link
Member

Cherry-picked for 3.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Apr 29, 2021
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.

3 participants