stride
stride copied to clipboard
stride3d
Consider moving to _privateField to follow the official C# style guide and reduce naming collisions
Question and/or Comment
Hey there recently when playing around a bit in the stride source code I noticed that Stride currently uses camelCase for private fields. This makes it very annoying to work with parameters with the same name.
To work around this issue often parameters are suffixed or strangly named values (see CommandList as an example) I would like to ask if it is possible to loose this restriction or move to the standard style guide of C# and use _privateField instead. As far as I can see it there wouldn't be compatibility issues as I only suggest naming changes for private fields.
See here for reference https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/coding-style/identifier-names
I like your suggestion, especially since game engine code can become extremely long—spanning multiple screens. This would be also particularly helpful when dealing with many variables.
I think this is a great idea. I also always found private vars a bit off in Stride due to this. This will be very tedious work and the only downside is it will cause merge conflicts with 99% of PRs but I still think its worth it.
Probably, this approach would make sense when:
- New additions are being added to our codebase
- The existing codebase is being updated for other reasons
- Large sections of code would benefit from such an update
In other words, it might not make sense to change everything solely for the sake of this suggestion, unless majority is already done? 🤔
Probably, this approach would make sense when:
- New additions are being added to our codebase
- The existing codebase is being updated for other reasons
- Large sections of code would benefit from such an update
In other words, it might not make sense to change everything solely for the sake of this suggestion, unless majority is already done? 🤔
- If it were only done on new PRs then we would be relying on the people making those PRs to be aware of this wanted change.
- The changes would be too infrequent for this to ever come to light and ever be finished.
- I think if the goal is to be known as the C#/.NET engine, following code standards set by the language would be a benefit and to not change the areas just because not all the code would benefit feels wrong to me.
IMO this change will be like ripping off a bandaid and just getting it done so that it doesnt continue to be an issue in the future. Of course saying that, someone will have to take this one either project by project or all at once.
Project by project is also good idea. I would add then also removing empty spaces at the end of anything :)
I suggested December 2023, be a month of Docs, maybe 2024 Q1, could be a spring of minor refactoring and code clean up 🤣
Not sure if we want to rename all parameters as well or only change the private fields. The second option in rider is just a button click. It can do that automatically for a whole project in a few seconds.
The biggest issue I see here is with potential issues in Asset serialization where we might have private fields serialized with their current name.
I also like this comment very much
I think if the goal is to be known as the C#/.NET engine, following code standards..
I agree, especially because adhering to C#/.NET coding standards makes the codebase more accessible and easier to understand for developers familiar with those standards. It's much simpler to follow established guidelines.
Of course, this would mean that the current maintainers would need to familiarize themselves with these standards, as it would represent a change for them.
I think a global .editorconfig could go a long way.
On a similar note, dotnet format could be enforced in a CI check before pr merges.
On another similar note, so could https://github.com/belav/csharpier if whitespace formatting is something we want to enforce.
.editorconfig is the way to go I think as it is supported without any extensions in VS, VS Code as well as Rider. I personally use one for every project I do and it is very easy to setup. It also ensures all devs opening the prohect to use the same settings which increases code quality and avoids whitespace diff changes.
I see Stride also have .editorconfig, shouldn't it be in the /build folder?
I think .editorconfig is in the right place, maybe we should consider deleting all stylecop leftovers, because project was droped as far as I know.
Duplicate of #1371 - this is a non-issue, converting the codebase will introduce tons of unexpected issues and require a lot of work on the part of the contributor and even more so on the reviewer, all this to kill git blame while not actually providing any consequential gain.
This makes it very annoying to work with parameters with the same name.
You should never use a prefix as the sole distinction between function local and class local member, it is impossible that the both of them have the exact same context, use a name that clarifies the intent of the function local member better.
Closing the other one, but I very much feel like we should close this one as well. While I do prefer dotnet's style, it's too late, this is just a waste of time.
You should never use a prefix as the sole distinction between function local and class local member, it is impossible that the both of them have the exact same context, use a name that clarifies the intent of the function local member better.
How about constructor injection?
public class MyClass
{
private readonly SomeService _service;
public MyClass(SomeService service)
{
_service = service;
}
}
@Eideren that is just not true the _ helps tremendously to distinguish fields from variables and parameters. Depending on your IDE you can't tell if it is a parameter and I for example had several bugs in my changes because I mixed up parameter and field value. Now beeing part of the .NET Foundation I would higly recommend to follow the official coding standard as it should have never been this way anyway. 95% of the Gitblame shows you Xenko Team anyway so there is not really a lot of information to loose and at some point you will trim the history anyway I guess.
We could think about using a git history rewriting tool to do the changes this way we rewrite what has been done in the firstplace without loosing any history.
Some arguments for:
- Code that is formatted the same way becomes easier to read once you understand the formatting.
- Cleaner code base is enforced with proper
.editorconfigand analyzers, increasing the signal-to-noise ratio of the code.
Some arguments against:
- Increases barrier to entry for contributing because you have to understand the required formatting.
- Slower iteration process if steps to enforce the formatting are added to CI.
Anecdotal data: I use all of the tools I mentioned here at $work, and while initially it messed up the git history a bit, I've come to love it. I think it makes PR's easier to read as well.
You should never use a prefix as the sole distinction between function local and class local member, it is impossible that the both of them have the exact same context, use a name that clarifies the intent of the function local member better.
How about constructor injection?
public class MyClass { private readonly SomeService _service; public MyClass(SomeService service) { _service = service; } }
primary constructors would lead to go back to non underscored fields for such things
How about constructor injection?
It's just a method, same thing applies.
95% of the Gitblame shows you Xenko Team anyway so there is not really a lot of information to loose and at some point you will trim the history anyway I guess.
That's not really a strong argument in your favor, that '5%' is still very much worthwhile, and we may trim history far off in the future, but this is not nearly the same thing.
I'm not going to stop you working on this, just know that you will have to offer your help for people working on forks like Kryptos and Phr00t, as well as ongoing PRs as this change will create merge conflicts for them. It's very likely that no maintainers would have the time or motivation to look over your changes, there's no way we merge such a huge amount of change without reviewing and testing it properly.
Regarding the argument:
Increases barrier to entry for contributing because you have to understand the required formatting.
As this discussion shows, since we are bringing this to the table, some of us new contributors are already familiar with the mentioned standards and are advocating for them.
For new contributors who aren't familiar with these standards, they'd still have to learn the project-specific formatting anyway 🤷♀️🤷♂️.
The real change would be for our dear existing maintainers, who are being nicely 😂 nudged toward adopting the suggested standards.
While I definitely think auto-formatting and hard enforcement of the standard is the best choice, I do recognize that it is not a one-sided clear winner. Another case to be considered as an argument against is
- Updates to the official style guide will again mess up the git history
I think we can adjust our style going forward without rewriting our git history, and consider a project-wide re-format once we make enough style changes to warrant one.
Let me start by saying that relying on "official" guidance for private members isn't exactly a strong reason. A much stronger one is to have a consistent formatting across the code base.
I want to point out that there's a lot of variance in people's preferences (e.g. I much more prefer an explicit this to any naming prefix). The way teams I worked with usually deal with it is that they use an editorconfig file with naming convention enforcement (it's a compilation error to not use the correct naming convention).
I rarely have an issue navigating stride code base and very often my navigation happens on GitHub itself where there's no IDE support for determining what is a field and what isn't. As Eideren points out - in most cases there should be a contextual distinction between names.
Having said that, given we have potentially many new developers coming in to look at the code base and they have opinions and habits, we do need to agree on a common middle ground.
If we end up going this way I would propose the following:
- If you're introducing a completely new file, you should follow the underscore notation for private fields
- If you're modifying an existing file you should use the convention specified within it (unless your changes anyways impact every usage of the private members and there would be no blame information left anyways).
But how do we enforce it? Mention it explicitly in every PR review? I don't think editorconfig format is capable of directing changes at new files only.
As with everything, if there's no enforcement or such enforcement takes more energy than it's perceived to be worth, then it's not going to happen consistently.
And we would end up with an inconsistent code base for many years, as rewriting it all takes time.
As Eideren points out - in most cases there should be a contextual distinction between names.
It's more mentally taxing to figure it out by context, than it is to be able to just read it from the underscore. That might not matter though, some people don't mind and if you're reading through a small pr, chances are that a little extra mental work here and there isn't gonna stop you. I still think it was worth pointing out though.
A much stronger one is to have a consistent formatting across the code base.
💯 ! There's a lot of mental overhead in navigating a codebase with structural inconsistencies, and even if the naming of private members is a small thing, every little bit adds up.
Thanks for all the feedbacks!
Just an additional personal opinion and history:
I never liked _ much because I feel it breaks the typing flow for local variable that are accessed very often (need shift key) and it felt like a remnant of time without IDE/intellisense/resharper.
And the idea was that IDE can easily highlight/differentiate variable vs field. So we used this. in case of any ambiguity (i.e. function parameter).
Also there was no clear standard at the time, I remember many code base with _ and many others without.
OTOH, I agree that if not careful in a manual refactor (i.e. removing a field), the code might still compile and suddenly act unexpectedly on the parameter.
Happy to go toward any consensus we might reach, I won't put any veto on it. However, as mentioned several time, it's a considerably large change breaking history/branches and there's also the issue of consistency, so I think it would have to be a clear improvement & majority of people in favor to be worth doing.
My two cents to all of that. I havn't planned a big discussion like this. I would vote against partial changes as it introduced wrong assumptions when reviewing a file where we didn't change to underscore yet.
this. would work for me as well but we should then enforce this using the editor config and also disable the warnings about ambuiguity of parameterName vs fieldName. For me at least Rider complains a lot if I use the same name for the parameterName and fieldName even though it is perfectly valid.
My biggest issue is especially with parameterName = fieldName as it is SO easy to forget a single this. and introduce bugs which might not even be catched in a review. That's why I personally love the idea of the underscore prefix and strongly recommend it.
But for me at least it really comes down to a ALL or NOTHING descision. Because as mentioned above different conventions per file is a REALLY bad idea imho.
I also want to add to the comments from @xen2 that the IDE easily shows you the difference between a field and a parameter. This is not true. Many ppl use different IDEs and or different Themes and the coloring behavior is different in each of them. In rider the parameter is a slightly lighter gray compared to the field and I don't want to change my personal appearance just for the Stride repo. Another good example is github diffs because there we don't have different coloring for fields vs parameters at all so especially for reviewing it is very difficult imho.
Here is also a talk from Nick from a few weeks ago. The topic is different, but he shows how it looks like within Rider which is a very popular choice nowadays: https://youtu.be/QdfAOVk77v0?si=zx86CWJ2mj7ISS5C&t=318
If the decision ends up being doing a change, we should probably also decide what other style/formatting settings Stride should have, and do them in one fell swoop.
I don't know if this is the case anywhere in the code, but we should also remove any occurrences of the name field in anticipation of https://github.com/dotnet/csharplang/issues/140.
I don't know if this is the case anywhere in the code, but we should also remove any occurrences of the name
fieldin anticipation of dotnet/csharplang#140.
The proposal says the field keyword would be contextual (like value in the setter) so it shouldn't be an issue outside of that context. Otherwise it would be a significant breaking change.
This would completely break any changes I've made to the code for my game... I imagine I'm not the only one in a situation like this (I know phr00t has a nice fork).... so I'm giving a strong "pls no"
edit: thanks @Eideren I see you have noticed our kind
