Remove underscores in front of C# "lifecycle" methods to match the language's conventions #14466
Comments
|
CC @neikeq @Hinsbart |
|
I guess we could remove them. No idea if it may cause conflicts though, it's a matter of trying. It's also a simple task, changing it here should be enough: godot/modules/mono/editor/bindings_generator.cpp Line 1644 in 75d0aeb |
|
Not that I mind the underscore, but it probably would be less weird to not have it :) If nobody has already taken this task I'll go ahead and tackle it if nobody else wants to do it. |
|
@neikeq There are a few naming conflicts for some of the method names. For instance, there's a "Get" method on Object, but then there's a virtual "_Get" method. So we end up getting a conflict when the underscore gets removed. There are a number of solutions that could be used, but the first one that comes to mind is to replace the underscore with "On". So, "OnReady", "OnProcess", etc... I think this would be more consistent/expected compared to other API's. I tried this and everything is working conflict free. Ideas? Thoughts? |
|
We would need to hear the opinion of users about that. Personally, I don't have anything against it. Is there any name that doesn't sound right with the "On" prefix? |
|
Slightly dumb example given you couldn't simply override public class Button
{
private string _text;
public string Text
{
get => _text;
set
{
string oldText = _text;
_text = value;
OnTextChanged(oldText, _text);
}
}
public event TextChangedEventHandler TextChanged;
protected virtual void OnTextChanged(string oldText, string newText)
{
TextChanged?.Invoke(this, new TextChangedEventArgs(oldText, newText);
}
}
// And people typically did
public class FancyAssButton : Button
{
private string _textToDraw;
protected override void OnTextChanged(string oldText, string newText)
{
// Oops, didn't call base, but who cares, right? Right.
_textToDraw = ConvertTo1337Speak(newText);
}
}So it has been abused over time to become exactly what we want it to be, a place to put custom code for handling events. It's definitely better than the underscore which outright violates C# naming rules. Let's not be as bad as Unity in that matter (with it's ugly lowercase public instance members) :) Plus I can't think of a better prefix otherwise, except |
|
They seem to work, but here's a full list if anyone wants to chime in: AStar.OnComputeCost |
|
@NathanWarden @RayKoopa The only methods where the Edit: I think same applies to |
|
Sounds totally fine to me. OnIsTileBound is what sounds horrible to me. |
|
@RayKoopa Uh. Didn't saw that one. |
|
@RayKoopa Yeah, haha, that one is a bit odd :) I'm almost certain we could make a blacklist of exceptions for this. @levrik I agree those sounded slightly odd to me too until I thought about what was really going on. IE. they are really about the event of the frame or physics being finished and here's how long they took (delta). I'm fine with the names, but for sake of clarity of what I'm saying, is that they could have been more accurately called OnFrameProcessed or OnPhysicsProcessed. @levrik @RayKoopa @neikeq Since we'll most likely need a blacklist of some methods, what do you guys think about Process/PhysicsProcess having "On". I'm casting my vote for having "On" for them since they are technically events, but am also not dogmatic about that either. Finally, @neikeq if a blacklist sounds good, do you have a preference where this should be and data type? The most obvious would just be sticking it right in the function as an array/hashtable. |
No blacklist please. You can do what you want to make the C# bindings more idiomatic, but it should all be automated conversion that anyone can perform in their heads to go from GDScript to C# and back when familiar with both languages. Renaming The aim is not to be like "ah, this Godot API is awkward, let's hack the Mono bindings so that it's less awkward". If an API is awkward, it should be fixed for everyone or noone. |
|
@akien-mga I don't think @NathanWarden proposed to change that in the C# bindings only. |
|
I agree on not having a blacklist, that makes everything needlessly complicated. If the prefix is bad, we need another one. If you feel crazy, even something like |
|
@akien-mga What @levrik said is what I meant. It wasn't a name change proposal. I was only using the name OnFrameProcessed as an example to show that the Process function is actually an event and that it could have just as easily been named OnFrameProcessed, therefore calling it As far as IsTileBound... is this even a proper name for this function? It looks like it should actually be called TileIsBound or simply TileBound because this is the method signature. Notice there are no booleans despite it's name implying them. @akien-mga Now this time, haha, I'm possibly proposing a name change :D
I think it's misnamed? Can somebody who's familiar with |
That would be a bug in the method binding, it's a virtual method and it is supposed to return a boolean (well actually a
|
|
The wrong |
|
Ah, okay, thanks for clearing that up :) So, just to be clear, the blacklist I was talking about would be to not put "On" in front of certain methods like IsTileBound. The rest would be OnXxx, IE. OnProcess, OnPhysicsProcess, OnReady, but then you'd have IsTileBound instead of OnIsTileBound. Are you fine with that, or do you think that will be problematic? Out of about 40 or 50 methods we literally have 1 that I'm not sure there's really any other prefix to put that would make sense on all methods. Putting |
|
IMHO no blacklist, not even for a single method. A developer would wonder why all other methods handling Godot stuff are prefixed with |
No, there are a lot more than that. Any virtual method bound to scripts that returns non-void is basically not an event. See e.g.: Those are not events, they're methods you can override to return a float which is used in AStar's algorithm. You can define your own cost estimation and computation logic using those methods (in GDScript, So that's to see with @neikeq who knows this stuff better, but it might be worth separating the bulk of As @RayKoopa says, there should not be any blacklist/no exceptions. If the virtual methods have different natures, they should be better separated at the engine level, not in the bindings for each language. |
|
Yep, I agree. Then, I think it would be best to just leave it as underscore? :/ (edit: It does keep it consistent with GDScript, and therefore the documentation as at least one positive thing)
But, I'll be out for about half the day. So if you guys can think of something better, when I get back I can just drop it in and I can generate a new list that I can post so we can see if it works :) |
|
I don't think I think this is a good opportunity to review them and break compatibility if need be. |
I agree, but it should be discussed in its own issue :) |
|
OnGet/OnSet are weird to me. Or in general, I always saw On* methods as event/signal handlers, and as such, methods that don't return a value and that are called by a notification/event system... |
|
Also, underscore in front of methods often means that the method is not public, which is specified already in C# (and not in GDScript), so maybe removing the undersore and making the method |
|
I think we should just keep the underscore, it doesn't look bad to me and makes it more familiar for existing Godot users. That being said, we should ask users about this to have a better idea of what they think (probably in the facebook group where most people is). |
|
@Zylann I definitely would have liked just the method names too, but that won't work without changing several Godot method names, which would also affect GDScript, which means we'll need to get more people involved than just those of us interested in C#. I agree with @neikeq. I think we should just keep the underscore. It isn't ideal, but it really works just fine. I personally never even noticed a problem with it until it was brought up, haha. But, maybe that was just from using GDScript with Godot up to that point. :) Plus, we could always revisit this in the future. IE. If enough people want it changed we could change it in something like Godot 4 (or a Godot 3 point release if it makes sense). I think it'll be far more obvious how we should proceed after Godot 3 is released whether this is an issue for the broader community (Facebook/Twitter/etc) or not. |
|
I'm all for removing the underscores, as many people on this thread already know I tried, but the cure for this seems to be more troublesome than the disease. |
|
Based on everything I know about this issue, that removing would be a pain and possibly have inconsistencies, for a minor style benefit, I vote to keep the underscores. By the way, I personally prefer the underscores to |
|
What frustrates me is that people call it a "mild style benefit." The fact that very few people seem to understand the importance of conforming to standards and conventions (and standard conventions) is this most disappointing part of this whole thing. |
|
If there's significant practical hurdles to fix this issue, I agree that we should leave it as it is. I'd like to see it fixed though but based on what I read in this thread, it seems it won't happen unless someone will take the job him/herself and makes a PR. I believe the majority of us agree that it's better to fix it so our codebase could be more conforming to the standard coding convention by now. As such, if we need to discuss this matter further I guess it better be on the technical side, so we can hopefully find a way to overcome whatever obstacles for fixing this before 3.1, if it's feasible at all. |
|
@jonbonazza The standard which we are conforming to, in this case, is the Godot API. |
|
@aaronfranke But in this case, it's a standard that is mostly pointless in the context of C# which also conflicts with the widely used conding conventions of the language, so I agree with what @jonbonazza said in principle. And if we are to regard whatever coding convention that Godot API suggests, probably we shouldn't have tried to convert all snake_case names to PascalCase ones in the first place. But as I said, I can see how fixing the issue involves some real work while there are other issues which better be fixed before 3.1. So I just hope someone who knows internals of how glue generation works to take some time to contribute. |
|
If anyone wants to give it a try they can reference my code here, although it's a bit outdated: https://github.com/NathanWarden/godot/tree/cs_virtual_method_rename |
|
Guys, the problem is not implementing it. I would gladly implement it myself if there was anything to implement. I explained what the problem was. If you want to see this done, you should try proposing a solution for the conflicts. |
|
@neikeq I think I understand the problem, since what I meant by 'implementing' certainly includes finding a suitable way to resolve the Maybe we could get a more clear idea of the problem if you could explain the difficulties involved in that specific case. So, I'd like to ask what would happen if we simply remove |
Like I said previously, I really don't understand the need to not deviate one bit from a standard convention but, as explained above, that's not the reason this is not changed. Many changes were already made to please C# users, even though I may not agree with many of them. |
|
@NathanWarden shared an screenshot of the errors he gets when removing underscores in thi comment. Reading that comment, I see why there was the confusion above about these methods being internal. The list of conflicts seems to be:
At first glance, one would say we should have a single method. There is a problem in C# with sharing the same method for both get and _get though, see: Engine's |
|
Thanks for the explanation. Then, it seems that the heart of the matter is that the native method is invoking its virtual counter part (as I also suspected in my comment above). In that case, the only remedy I can think of is to use However, even though using such a prefix is quite common, it's not a part of any conding standard that I know of. On the other hand, names of non-private methods are covered by the official coding guideline from MS, which clearly states they should follow PascalCase naming. So, using something other than But simply using either of those proposed alternatives for all cases probably would result in unnatural names. For example, while Maybe it'd make more sense if we can extend the ClassDB format so that we can specify different names for a specific binding by hand (and possibly with other informations, like what additional interfaces it'd extend, and etc - i.e. #17791). But if must rely on blind replacing of auto generated method names, I'm not sure if it'd be worth the hassle at this point. So I have to agree it'd be better to leave it as it is, even though I'd like to see Godot's API to be as much standard conforming as possible. |
|
I agree with @aaronfranke; C# methods should be consistent with GDScript methods. If it is stupid to have an underscore before each lifecycle method, what about renaming the GDScript methods also? Unpopular opinion: How about we implement function overrides in GDScript and just use |
|
In my opinion, it would be better C# methods to be consistent with the official convention of the language rather with another language which follows different rules. I agree that we shouldn't make it confusing for people who may switch between the two languages to use Godot. However, I don't think it'd be too difficult for them to see which GDScript method to correspond with a C# one, as long as there's a consistent rule (e.g. snake_case to PascalCase). As to lifecycle methods, it seems that the only reason why GDScript adopted such a convention is that it does not have |
|
@mysticfall for example signals are named I think we should follow the C# style guidelines and use an example would be
their editor counter parts would remain without |
|
Is the event Action<string> TextChanged;
void OnTextChanged(string text) {
TextChanged?.Invoke(text);
} |
|
its used for the functions that implement the logic |
|
In my opinion, On[X] makes sense because _ready etc are like engine events in a broader sense (which makes it a nice hint towards their intention too). And Regular methods are more likely to be imperatively named which means 'Ready()' Set() Go(), are free to be used by programmers. Plus regular event subscriptions are a little less likely to be overrides at the same time. |
Any reference? From what I've seen, this is not the case, and the convention is to use it on methods that raise the event as in my example.
The motivation for getting rid of the underscore prefix is that it looks out of place when considering standard conventions, so it doesn't make sense to change it to something that goes against standard conventions as well. |
|
I'll give a try at removing the underscore soon and see what the resulting conflicts are. If the conflicts are very few and for unimportant methods then I think we can move forward with the change, only keeping the underscore when there are conflicts. |
|
Today the naming is internally consistent. Don't sacrifice that for a little syntactic sugar. |
Calinou
changed the title
|
Closing in favor of godotengine/godot-proposals#757, as feature proposals are now tracked in the Godot proposals repository. |
Not sure if this was brought up before already but I'm confused by the naming of the "lifecycle" methods (e.g.
_Ready,_Process) in C#. They also have the underscore in front like GDScript.So far as I understand why this was needed in GCScript I'm not sure if it's needed in C#. It feels kinda weird to me since we already have the
overridekeyword in front.The text was updated successfully, but these errors were encountered: