ProwlEngine
Fix/build warnings 2024 09 15
Attacking #158
fixes for:
CS0108: 4 CS4014: 4 CS9191: 3 CS8619: 3 CS0649: 3 CS8765: 2 CS8767: 2 CS8620: 2 CS0659: 1 CS0661: 1 CS8714: 1 CS8607: 1 CS8509: 1 CS1998: 1
from 529 to 467 compilation warnings (not considering DotCast nor Veldrid)
NOTE: If one wants to focus on solving these issues, I suggest to adding
dotnet_diagnostic.<CODE>.severity = errorinside.editorconfig. Your IDE might immediately point to errors. It not, try compiling the code and check the log
It might make more sense to disable the Readonly warning. As there's a ton of cases with the Readonly's where it doesn't make a whole lot of sense. Including a lot of public API intended for end users to change in runtime Or ones that frequently change via Reflection using the Inspector in the editor
For example almost all the Components/Monobehaviour shouldn't have readonly fields, since the whole point of them are settings for the components.
Understand the point. Some thoughts:
- Dictionaries/Lists generally are not recreated but rather manipulated (add, remove items)
- Fields, unlike Properties, are not meant to be public settable. f327527 only changes fields.
- The static code analysis shows that any of these fields were changed inside the code. Of course, if there were meant to be used by the user, we could change them to properties.
In general, readonly allow the compiler to make A LOT of optimizations and also to raise aware when one tries to change them.
Paths forward:
- Maintain the changes and revert case-by-case (when ones code depend on them)
- Revert some classes
- Revert all the commit
The issue with switching to Properties is that moves away from the Unity API. Unity doesn't really even support properties-only fields, our serializer also doesn't handle them properly and it would be a reasonably big change for newcomers to c# who only know c# through Unity.
I think the read-only one makes sense to remove for the time being, What do you think @sinnwrig
One thought I have is maybe not worrying about Unity only devs? We would only be perpetuating the problem.
It's problem going to bite us in the future, but for the time being, I'm reverting it. There are still a lot (200+) other warning to attack.
One thought I have is maybe not worrying about Unity only devs? We would only be perpetuating the problem.
I mean a huge part of prowl is to be a unity alternative, so it's main audience by far will be unity devs. On top of the fact that it's not really a huge problem to perpetuate in the first place. Its less a problem and more a design choice in my opinion.
ok. Commit reverted.
I believe Unity did some choices back in old days due lack of resources. Joachim (Unity ttech chief back then) repeatedly said there are several design mistakes that now they kinda have to live with.
Their serializer is sub-par. Sirenix (Odin) serializer is much more capable.
Using public mutable fields is a very bad practice. Basically all Static Analyzers will complain: Visual Studio, Rider, Codacy, Qodana, etc.
Sicne drag-and-drop replacement is virtually impossible, I believe we could have peace of mind on taking some things in other direction. As long it's well documented and explained to users.
The issue with switching to Properties is that moves away from the Unity API. Unity doesn't really even support properties-only fields, our serializer also doesn't handle them properly and it would be a reasonably big change for newcomers to c# who only know c# through Unity.
I think the read-only one makes sense to remove for the time being, What do you think @sinnwrig
The readonly warnings should only be appearing because of C# style recommendations, correct? If so, it would probably be feasible to revert the warnings back to suggestions in the .editorconfig, since the compiler can't be aware of how a user and the serializer might interact with it. On top of that, the warning seems a bit like premature optimization, since the performance boost gained by setting fields to readonly is negligible in most cases unless it's in perf. critical code, in which case it can be optimized manually later down the line.
ok. Commit reverted.
I believe Unity did some choices back in old days due lack of resources. Joachim (Unity ttech chief back then) repeatedly said there are several design mistakes that now they kinda have to live with.
Their serializer is sub-par. Sirenix (Odin) serializer is much more capable.
Using public mutable fields is a very bad practice. Basically all Static Analyzers will complain: Visual Studio, Rider, Codacy, Qodana, etc.
Sicne drag-and-drop replacement is virtually impossible, I believe we could have peace of mind on taking some things in other direction. As long it's well documented and explained to users.
On the public mutable fields, it seems a bit harsh to remove them completely because of bad design practice, since for many cases, having simple, raw, exposed values without a get/set (even if it's auto-implemented) is useful. If it's possible, it would eventually be nice to support [SerializeField] on property get/set. However, for most cases, something like [field:SerializeField] attribute is good enough to let the serializer get to an auto-implemented property.
In addition, while it might be good in the context of an organizational project, the engine should not be enforcing good practice or style on users, and the decision should ideally be the developer's to follow their own style guides and practices.