godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.11k stars 69 forks source link

Rename C# override methods to use `On` prefix instead of `_` #757

Open Shadowblitz16 opened 4 years ago

Shadowblitz16 commented 4 years ago

Describe the project you are working on: Tower Defense Game

Describe the problem or limitation you are having in your project: OCD related to C# naming convention

Describe the feature / enhancement and how it helps to overcome the problem or limitation: Rename node signals and hooks to use On prefix instead of _

example void OnReady() {} other then.. void _Ready() {} Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams: It would just be renaming all of the hooks and signals to follow the C# naming convention in C# scripts.

other languages would not be effected

If this enhancement will not be used often, can it be worked around with a few lines of script?: This would be used every time someone looks at your code and no it can' be worked around.

Is there a reason why this should be core and not an add-on in the asset library?: it would just be a change in the mono library

Calinou commented 4 years ago

Related to https://github.com/godotengine/godot/issues/14466.

Calinou commented 3 years ago

@neikeq What do you think about this? This is something we should look into before Godot 4.0 is released, as this is a breaking change.

neikeq commented 3 years ago

I'm against the On prefix because the motivation to remove the underscore is to follow the C# convention and the On prefix also goes against convention. My conclusion was that we can probable remove the underscore except in cases where doing so would cause a conflict with another method (e.g.: Object.Get and Object._Get).

This has low priority in my list, so if anyone else is interested in make a PR, feel free.

Shadowblitz16 commented 3 years ago

@neikeq Microsoft did it in winforms so it doesn't go against C# convention.

On is used for virtual event invokers in winforms. signals wouldn't use it they would use something like.. Ready instead of OnReady

btw I don't think the godot editor signal names will change so its more of just the implimentation of signals and the callbacks..

for example the node's _ready would be called OnReady but connecting a ready signal to a method, it would be Ready

neikeq commented 3 years ago

On is used for virtual event invokers in winforms.

Exactly. It's used for event invokers, not for callbacks. That's why I say it goes against convention to use it for callbacks.

Shadowblitz16 commented 3 years ago

The invokers are virtual though and people use them as callbacks. for example..

//Winfroms
public override void OnMouseDown(KeyboardEventArgs e)
{
   base(e)
}
//godot
public override void OnReady()
{
   base()
}

looks the same to me.

Flavelius commented 2 years ago

OnX also seems like a good fit to me because it is reactive to an engine invokation. It's also something i stumble over in unity with its imperative Start and Update methods, which are way too common commands, that object methods that act on their data usually also tend to be. This means if one wants to have a commandable processing unit - Process() would have to be evasively named for example DoProcess().

P.S. Interestingly, unity does follow the OnX style consistently for almost all other engine callbacks : https://docs.unity3d.com/ScriptReference/MonoBehaviour.html

cphillips83 commented 1 year ago

I would just like to add another view point. As someone that has been learning Godot, I have used the _ a lot to just get a "sense" of what the class is. The same thing could be said with the change to On but not everything is OnXYZ.

_ provides no inferred meaning or intent and feels agnostic which is a good thing when it needs to fit universally.

lumirel commented 1 year ago

It would be nice to see some kind of fix for this for the 4.0 release.

YuriSizov commented 1 year ago

This is not a bug, so there is no fix. As a proposed feature, there is a motivated rejection from the area maintainer above and there are no plans to make the proposed switch. In fact, a lot of effort was put into synchronizing C# APIs with the engine lately, so any change of this kind in future is unlikely.

lumirel commented 1 year ago

I see. I've read through the history of this proposed feature. I used the word fix specifically because I consider keeping the underscores one of the few objectively wrong choices. Underscores are only ever used for private member variables. There's few, if any, exceptions to this from my experience.

I assumed something would have been changed regarding these methods for the next major release considering all of the effort that has been put into synchronizing C# APIs. That one day I would just see a change on the blog. I thought this might have just been a simple oversight. Just my feelings about the whole thing. Apologies. I'm new here.

YuriSizov commented 1 year ago

I assumed something would have been changed regarding these methods for the next major release considering all of the effort that has been put into synchronizing C# APIs.

Well, then we are staying consistent with the engine. Virtual methods that you implement to extend or override certain behaviors are also private, and are prefixed with an underscore in both the engine code and the first-party scripting language GDScript. When a method is supposed to be called from the outside, it has a public counterpart, which doesn't have a prefix, but which you also do not want to override. Some methods are only used internally, such as _ready.

lumirel commented 1 year ago

Well, then we are staying consistent with the engine. Virtual methods that you implement to extend or override certain behaviors are also private, and are prefixed with an underscore in both the engine code and the first-party scripting language GDScript. When a method is supposed to be called from the outside, it has a public counterpart, which doesn't have a prefix, but which you also do not want to override. Some methods are only used internally, such as _ready.

Good to know. However, as a long time C# developer its just confusing because where and when to use underscores is a long established convention. But thanks for your time and the explanations. I've learned that Godot is going to prioritize maintaining parity with the engine over using long established C# conversions. I will adjust my expectations in the future.

neikeq commented 1 year ago

Some clarification:

Methods that begin with underscore are similar to callbacks. AFAIK the C# convention is to use On for event raiser methods. Therefore it would go against convention to use them here, and if we're going to go against convention, we may as well stay with the underscore to avoid confusion (although, as I said above, it could be possible to get rid of the underscore for some methods).

If we use On in the future, it's more likely going to be for generated signal emitters, which are similar to event raisers.

Flavelius commented 1 year ago

https://stackoverflow.com/questions/18703490/does-the-prefix-on-in-the-name-of-the-c-sharp-methods-imply-anything#18703527 but the first comment by Marc gravel here also backs the convention that even more applies here; for intercepting virtual/abstract event handlers in subclasses

lumirel commented 1 year ago

Some clarification:

Methods that begin with underscore are similar to callbacks. AFAIK the C# convention is to use On for event raiser methods. Therefore it would go against convention to use them here, and if we're going to go against convention, we may as well stay with the underscore to avoid confusion (although, as I said above, it could be possible to get rid of the underscore for some methods).

If we use On in the future, it's more likely going to be for generated signal emitters, which are similar to event raisers.

This makes a lot of sense. Honestly, I don't think there needs to be a convention for methods that are similar to callbacks but not actually. No underscores. No "On" prefix. However, if I understand this issue and similar ones, just removing the underscores introduces naming conflicts, correct? Solving those conflicts would mean losing parity with the engine, which is a high priority. Seems like being stuck between a rock and a hard spot.

JeroMiya commented 11 months ago

Some clarification:

Methods that begin with underscore are similar to callbacks. AFAIK the C# convention is to use On for event raiser methods. Therefore it would go against convention to use them here, and if we're going to go against convention, we may as well stay with the underscore to avoid confusion (although, as I said above, it could be possible to get rid of the underscore for some methods).

If we use On in the future, it's more likely going to be for generated signal emitters, which are similar to event raisers.

I agree on not using On as a prefix to avoid confusion with native C# event raiser conventions. Also agree with avoiding blacklists or customizable mappings - there should be a consistent automatable strategy for generating method names and resolving name conflicts. As to that, one resolution method would be to use the new keyword, but it should be used with caution as it may cause unexpected results.

Example:

class Foo { public new string ToString() => "Foo.ToString()" }
object willUseObjectToString = new Foo();
Foo willUseFooToString = new Foo();
Console.WriteLine(willUseObjectToString.ToString()); // C# Interactive output: Submission#0+Foo
Console.WriteLine(willUseFooToString.ToString()); // C# Interactive output: Foo.ToString()

If we're going to use a prefix, it doesn't have to make sense grammatically like On, it can just use, for example, something like Gd, as in GdGet. While slightly worse aesthetically, this has the benefit of not conflicting with any existing naming convention, and retains an overlooked benefit of the current underscores (setting aside its incorrectness). That is, in an editor with completion support (VS, Ryder, VS-Code, etc...), typing Gd will filter and pare down the list of methods in the complete list to just the Godot methods. It also sidesteps the issue of whether a particular generated method is a query or event handler. As a prefix, Gd would only indicate that the method is related to Godot. The tradeoff is just that it's a little ugly and doesn't read as well.

Aside from that, a potential alternative would be to use a verb prefix based on the signature of the method (i.e. use one convention for void-returning methods, another for everything else). Something like HandleFoo and GetFoo, except that one of the examples is just Get, which would amusingly result in GetGet. Dropping the prefix for query methods would require conflict resolution.

I do have a question, Object.Get was mentioned as one method that conflicts, but there is no such method. Object has GetHashCode and GetType.

As someone who has to do many many code reviews, I feel (perhaps unusually) strongly about making sure to follow the standard .Net naming conventions, as long as you can map the engine-to-C# names in your head (i.e. no blacklists). Unity's complete lack of respect for them has caused no end of quality control issues and confusion from new C# developers with Unity backgrounds moving to web/enterprise/infrastructure related C# coding fields. The C# naming conventions are an important feature of the ecosystem, as they improve readability of code and avoid common misunderstandings. Using underscores for public method names in particular would be a very disruptive habit to have to break outside of Godot. I don't relish the thought of developers copying this convention elsewhere.

YuriSizov commented 11 months ago

Using underscores for public method names in particular would be a very disruptive habit to have to break outside of Godot. I don't relish the thought of developers copying this convention elsewhere.

From the engine's POV these methods don't necessarily have to be public. You are never supposed to call them from the outside, they are only called by engine internals. Not sure if there is a limitation in the engine-C# interop, though.

Introducing an inconsistency with Godot's own APIs would be a big issue as well. Due to how C# is implemented we already have to use special naming and structures in various places, and these things always trip people learning the tool. It makes it harder to find the documentation and make sense of the tutorials. That's an unfortunate downside of having a general purpose language integrated with the engine. So some sort of a compromise is inevitable.

lumirel commented 11 months ago

Introducing an inconsistency with Godot's own APIs would be a big issue as well. Due to how C# is implemented we already have to use special naming and structures in various places, and these things always trip people learning the tool. It makes it harder to find the documentation and make sense of the tutorials. That's an unfortunate downside of having a general purpose language integrated with the engine. So some sort of a compromise is inevitable.

I'm not a fan of this reasoning. C# is heavily convention based. Violating those conventions also makes it harder to make sense of tutorials and for people to learn.

YuriSizov commented 11 months ago

@Reiku42 Well, it's either we compromise on following C# conventions so people can understand the engine APIs better regardless of the language they use, or we compromise on API consistency (which we already do and it leads to confusion and mistakes). This is an inherent problem with using general purpose languages if you are not designing your entire framework around it. Godot is not designed around C#, it can speak a variety of languages. So it helps our users to find the information faster and to translate answers found online to their language of choice.

Ultimately, this is about making C# fit Godot, not Godot fit C#, since C# is only a tool to access the engine API and the API design comes first.

lumirel commented 11 months ago

@Reiku42 Well, it's either we compromise on following C# conventions so people can understand the engine APIs better regardless of the language they use, or we compromise on API consistency (which we already do and it leads to confusion and mistakes). This is an inherent problem with using general purpose languages if you are not designing your entire framework around it. Godot is not designed around C#, it can speak a variety of languages. So it helps our users to find the information faster and to translate answers found online to their language of choice.

Ultimately, this is about making C# fit Godot, not Godot fit C#, since C# is only a tool to access the engine API and the API design comes first.

@YuriSizov I still don't like this reasoning. This could happen with any language Godot speaks. This is not specific to C#. Failing to follow a language's conventions is a failure to speak that language imo. If enough conventions are broken then it puts the onus on us, the users of Godot, to become translators for Godot.

JeroMiya commented 11 months ago

@Reiku42 Well, it's either we compromise on following C# conventions so people can understand the engine APIs better regardless of the language they use, or we compromise on API consistency (which we already do and it leads to confusion and mistakes). This is an inherent problem with using general purpose languages if you are not designing your entire framework around it. Godot is not designed around C#, it can speak a variety of languages. So it helps our users to find the information faster and to translate answers found online to their language of choice.

Ultimately, this is about making C# fit Godot, not Godot fit C#, since C# is only a tool to access the engine API and the API design comes first.

Developers with C# backgrounds typically expect the C# language bindings of a native/system module to differ slightly than the native counterparts or language bindings for other languages. This is true for other languages to a degree, but C# in particular because some of the naming conventions C# uses are less common in modern languages (PascalCased methods being a notable example). Therefor not only is it perfectly OK for the C# language bindings to not match the engine's internal naming convention or the naming convention used in other languages, it's actively preferred. I would not call it a compromise at all, but rather just a well made language binding.

Godot is not unique in this regard - I would say this for any .Net bindings for a native module or system call, and it is extremely uncommon (outside of game engines with C# scripting support, for some reason) to do otherwise in the wider .Net ecosystem. Following C# naming guidelines and "projecting" the native Godot API into C# in a way that is consistent with any other C#/Native binding you would typically see is absolutely the only correct way to go, I strongly feel.

Also, just keep in mind, C# Godot developers that are new to the engine will be starting out with C# examples and docs. And, when they're comfortable enough to look at GDScript examples, they won't have a hard time translating just because the name of a method in the C# bindings doesn't have an underscore prefix or it's PascalCased. The syntax differences will be the more difficult obstacle, if anything. Further, C# developers can write their scripts in an IDE, so they get full auto-complete dropdowns to help steer them in the right direction.

YuriSizov commented 11 months ago

Also, just keep in mind, C# Godot developers that are new to the engine will be starting out with C# examples and docs. And, when they're comfortable enough to look at GDScript examples, they won't have a hard time translating just because the name of a method in the C# bindings doesn't have an underscore prefix or it's PascalCased.

That's not the only difference that we have and not the only difference that we might need to have to satisfy your request. I'm also not talking about hypothetical issues. I'm referring to actual reports and questions from users who have missed these docs and had issues translating examples and answers given to them. So it's an actual real issue that me must consider.

JeroMiya commented 11 months ago

That's not the only difference that we have and not the only difference that we might need to have to satisfy your request. I'm also not talking about hypothetical issues. I'm referring to actual reports and questions from users who have missed these docs and had issues translating examples and answers given to them. So it's an actual real issue that me must consider.

I see, that's unfortunate. I still stand by my statement - I don't see game engine scripting language bindings as being categorically different from, for example let's say Silk.Net bindings for Vulcan/Metal APIs. And developers using Silk.Net have all the same issues w.r.t. sample code in other languages as you're describing, and yet nobody using Silk.Net would suggest not using .Net naming conventions just so the method names in the interop bindings match the native C versions.

However, your reports of developers having some difficulty should be taken seriously. I wonder if we could take a closer look at the issues that are being reported and identify areas where the documentation could be improved. I find the documentation currently does a very good job of showing examples side-by-side in GDScript, C#, and C++.

From your description, it sounds like the reports are primarily from developers receiving example code in GDScript from external sources, articles etc... Whatever route we take with naming conventions, I think developers will always have some level of difficulty translating scripts in one language to another. If they are trying to convert a GDScript example they see on the web to C#, that will be difficult regardless. Without advanced tooling for script conversion (I don't recommend this - I've written a couple of these and they are the most fragile things), the next best thing is good documentation spelling out the differences in detail.

Which you have already, here: https://docs.godotengine.org/en/stable/tutorials/scripting/c_sharp/c_sharp_differences.html#doc-c-sharp-differences and it's linked prominently in the beginner section of the C# scripting documentation.

So perhaps we can dig into the reports you're getting and identify specific areas where they were having difficulty. I would imagine it's the syntax differences that are the bulk of the difficulty, but I'm curious to know what your reports would point out.

Shadowblitz16 commented 8 months ago

Using underscores for public method names in particular would be a very disruptive habit to have to break outside of Godot. I don't relish the thought of developers copying this convention elsewhere.

From the engine's POV these methods don't necessarily have to be public. You are never supposed to call them from the outside, they are only called by engine internals. Not sure if there is a limitation in the engine-C# interop, though.

Introducing an inconsistency with Godot's own APIs would be a big issue as well. Due to how C# is implemented we already have to use special naming and structures in various places, and these things always trip people learning the tool. It makes it harder to find the documentation and make sense of the tutorials. That's an unfortunate downside of having a general purpose language integrated with the engine. So some sort of a compromise is inevitable.

I agree with the fact that alot of methods should be protected but I still think On should be used for callbacks rather then _

On is pretty basic change when it comes to the users perspective. C# users already use On instead of _ so if someone is already a C# user they will understand. If gdscript and C# ever get reworked to compile down to gdextension then docs can be generated for each language.