NRT Migration Phase 2 #324
Conversation
This was likely recommended by AI, but it breaks a number of tests.
| @@ -1316,9 +1313,9 @@ private void AddToHomographDict(ILexEntry entry) | |||
| { | |||
| string key = entry.HomographFormKey; | |||
| object oldVal; | |||
| if (m_homographInfo.TryGetValue(key, out oldVal)) | |||
| if (m_homographInfo!.TryGetValue(key, out oldVal)) | |||
There was a problem hiding this comment.
what if we just create a new dictionary here if it's null? I know it's not called today when it's not null, but if it was in the future then it should probably just be created
| @@ -103,7 +105,7 @@ public IAnalysis GetObject(int hvo) | |||
| return (IAnalysis)m_everythingRepos.GetObject(hvo); | |||
| } | |||
|
|
|||
| public bool TryGetObject(int hvo, out IAnalysis obj) | |||
| public bool TryGetObject(int hvo, out IAnalysis? obj) | |||
There was a problem hiding this comment.
I suggest for these TryGet methods we put [MaybeNullWhen(false)] on the out variable, then we can remove the ?, that says when false is returned obj can be null.
| @@ -2,6 +2,8 @@ | |||
| // This software is licensed under the LGPL, version 2.1 or later | |||
| // (http://www.gnu.org/licenses/lgpl-2.1.html) | |||
|
|
|||
| #nullable enable | |||
There was a problem hiding this comment.
yeah maybe let's not enable it here since we just had to put ! all over the place, I'm not sure it's adding anything
| private string[] m_saSegments; | ||
| private bool m_fSuccess = false; | ||
| private string m_sErrorMessage = ""; | ||
| private PhonEnvParser? m_parser; |
There was a problem hiding this comment.
I say we just new up the parser here. It seems like it will be created when it's needed, but there's no arguments required so we should just make it here so it's never null.
|
@hahn-kev, I know it's been a while since we last worked on this. The last thing we discussed was wanting to push this WIP so that you/we could determine how we want to proceed. This work is really all over the place, because I was trying to get various AI's to do what I wanted, finding minimal success with any. I may give this more effort today. I remember two major directions we were considering, though there are surely more:
I don't think fixing everything at once, which is where this PR currently is, is proving to be the most practical. |
|
I spent some time working with an updated version of Copilot today, feeding it some highly explicit instructions and focusing its scope. I was able to reach something pretty acceptable with the Build.Tasks project. I'm going to continue to apply this procedure to other projects, as long as it continues to work well |
|
So, at this point, we have a couple viable options:
|
|
@hahn-kev is this guy dead in the water? Last I checked, we needed something different than just a regular code review here. More of a discussion as to how we want to press forward. It may be easiest, given my current availability, to try to identify the portions of this that are acceptable and undo the rest. |
|
Yeah I think you're right. I think the changes made to public interfaces are where the real benefit is going to be found. Everything else will benefit internal LCM work, but will take to much time to be worth it for the time being. |
This change is