First iteration of nullable approach.#61
Conversation
JortPob/Common/Log.cs
Outdated
| { | ||
| case Type.Main: | ||
| mainOutput.Add(message); break; | ||
| mainOutput?.Add(message); break; |
There was a problem hiding this comment.
I kind of wonder why we need nullable checks in classes like this. It should be impossible for this to be null here unless someone added code above the Lort.Init() in main.
I guess there is no reason not to do this either as it has no downside. IDK just thinking out loud.
There was a problem hiding this comment.
It's because the compiler isn't that smart, unfortunately. You're right in that unless that call is missed or royally screws up in an unexpected manner, it should never be null. Unless someone foolishly added something that happens prior to that call from Main, which is really what this would cover.
In reality, this is 99.999% superfluous but it gets rid of the compiler warning.
|
|
||
| public readonly string job, faction; // class is job, cant used reserved word | ||
| public string job { get; init; } // class is job, cant used reserved word | ||
| public string? faction { get; init; } |
There was a problem hiding this comment.
What are the differences between get; init; and readonly ?
There was a problem hiding this comment.
The IDE hints make it easier to track usage. The real answer is it does technically have a difference in the styles of initializing the property, with init being a little bit more flexible. It's not a common situation, so it's mostly the IDE hints for figuring out usage of properties. https://stackoverflow.com/questions/62372422/what-is-difference-between-init-only-and-readonly-in-c-sharp-9 for reference
There was a problem hiding this comment.
One limitation of readonly I don't like is that when you have a subclass with it's own constructor you can't set readonly values of the parent class in that subclass constructor. That possible with init or the same issue?
There was a problem hiding this comment.
Yes, that is allowed, even if you call the base constructor:
There was a problem hiding this comment.
Oooo I could improve some things in a few places with that. Nice.
|
|
||
| essential = record.json["npc_flags"] != null ? record.json["npc_flags"].GetValue<string>().ToLower().Contains("essential") : false; | ||
| essential = record.json["npc_flags"]?.GetValue<string>().ToLower().Contains("essential") ?? false; | ||
|
|
There was a problem hiding this comment.
Good cleanup here. Been meaning to move all my parsing over to GetValue()
Thanks!
JortPob/ESM/Content.cs
Outdated
| int g = int.Parse(record.json["data"]["color"][1].ToString()); | ||
| int b = int.Parse(record.json["data"]["color"][2].ToString()); | ||
| int a = int.Parse(record.json["data"]["color"][3].ToString()); | ||
| int r = int.Parse(record.json["data"]?["color"]?[0]?.ToString() ?? throw new Exception("LightContent is missing value data->color->r!")); |
There was a problem hiding this comment.
With stuff like this which is guaranteed to never be null I kind of wonder if it's needed to have checks?
Like I get wanting them for consistency but it does seems superfluous. Fields like this in the ESM are rigidly required and so the ESM json will always have it.
There was a problem hiding this comment.
That makes total sense and this is just one of those areas where I'm not familiar enough with the input data structures to know if this should always be present. This approach will very quickly notify if there is an invalid input, but we could just as easily do ! instead of ? for null imperatives which will force the compiler to assume it won't be null (it can realistically still be null).
There was a problem hiding this comment.
Aight. If you want to you could update the PR and change any ESM json parsing to use ! instead. The differentiation is pretty easy here. Anything where I do not explicitly check null already is a required field in the ESM. I can clarify anything you are unsure about as well.
| .WithDegreeOfParallelism(Const.THREAD_COUNT) | ||
| .Select(node => { | ||
| Cell cell = MakeCell(node, esm); | ||
| var cell = MakeCell(node, esm); |
There was a problem hiding this comment.
I know it's weird but the use of var like this drives me crazy.
I think it's PTSD from working as JS programmer for too long.
There was a problem hiding this comment.
I get it, the reason is that technically MakeCell returns Cell? not Cell so with nullable enabled it causes compilation warnings. I could just write Cell? instead. At least compared to JS the compiler and IDE do know the type so none of the intellisense/IDE hints should be wrong or fuzzy.
There was a problem hiding this comment.
Yeah. I think for me it's a bit of a "stuck in my ways" kind of complaint. I just find readability is better with explicit types. I still use them sometime when writing out the type becomes a hassle like with KeyValuePair objects though haha
There was a problem hiding this comment.
Yeah I totally get it. If not for the IDE it can be a bit confusing, so reviewing a PR can get messier with them. I was being lazy so I may change it or I may not.
2 participants
First iteration as enabling nullable at the project level created 1300 warnings. Therefore, I'm enabling it locally to find nullable issues and work through them in chunks. I enable it in the individual files from there and disable it at the project level so as not to destroy everyone's build outputs with warnings.
The general approach here is to throw informative exceptions where null/missing data cannot be recovered. There may be some instances, specifically with arrays, where I put empty arrays as the default, but it possibly would be better to throw exceptions instead.