Add: SymbolLibrary - Delete Symbol #896
Conversation
Update: EditableSymbolProject.cs - expose new MarkCodeExternallyModified to force reload Update: SymbolAnalysis.cs - Add single-symbol analysis using TryGetSymbolInfo Update: SymbolLibrary.cs - Add UI for DeleteSymbolDialog Update: SymbolUiJson.cs - Skip orphaned child UIs whose symbol child no longer exists
pixtur
left a comment
pixtur
left a comment
There was a problem hiding this comment.
Wow. That PR is some serious work! Awesome.
I think it's 90% there but there are some style changes, that should be aligned.
The only real issues are:
- avoiding hard coded dependencies and references like that list of packages.
- being really aware of GC allocations (it's not a real issue here, but in the context of TiXL it's crucial for performance)
- using TryGetXX with [NotNullWhen(true)] if possible
- Using doc comments either with
or just /** comment **/
Only 1 is critical. Because some day somebody will add another package or rename a package and things will break without a clear reason.
| } | ||
|
|
||
| // Include projects containing depending symbols (force-delete case) | ||
| if (dependingSymbols is { Count: > 0 }) |
There was a problem hiding this comment.
Huh? If nullable is enabled, that dependingSymbols should never be null. Why not just test for dependingSymbols.Count > 0 ?
Are you using Rider? I think it would have also highlighted this line.
There was a problem hiding this comment.
dependingSymbols can be null, if it is null it will skip trying to do affectedProjectIds.Add for dependent graphs. (since there are none)
| foreach (var id in info.DependingSymbols) | ||
| { | ||
| if (allSymbols.TryGetValue(id, out var symbolUi)) | ||
| if (allSymbols.TryGetValue(id, out var symbol)) |
| IsLibOperator = isLib, | ||
| IsTypeOperator = isType, | ||
| IsExampleOperator = isExample, | ||
| IsT3Operator = isT3, | ||
| IsSkillOperator = isSkill, |
There was a problem hiding this comment.
I really don't like this, because:
- this implementation implies that a symbol could be in In Lib, Skill, T3 and other package at the same time. If at all, this should be an
enumto avoid this. - But actually, we just want to store a reference to the ops package. Add a single "packageName" string.
There was a problem hiding this comment.
I was thinking, there might be a Skill + Example later on?
| private static void GetNamespaceFlags( | ||
| Symbol symbol, | ||
| out string rootSegment, | ||
| out bool isLib, | ||
| out bool isType, | ||
| out bool isExample, | ||
| out bool isT3, | ||
| out bool isSkill) | ||
| { | ||
| var ns = symbol.Namespace ?? string.Empty; | ||
| rootSegment = ns.Split('.')[0]; | ||
|
|
||
| isLib = rootSegment.Equals("Lib", StringComparison.Ordinal); | ||
| isType = rootSegment.Equals("Types", StringComparison.Ordinal); | ||
| isExample = rootSegment.Equals("Examples", StringComparison.Ordinal); | ||
| isT3 = rootSegment.Equals("t3", StringComparison.OrdinalIgnoreCase); | ||
| isSkill = rootSegment.Equals("Skills", StringComparison.Ordinal); | ||
| } |
There was a problem hiding this comment.
Yeah, this was a little too much of a "hack" - I was avoiding modifying LibraryFiltering - which relied on isLib - This is now fixed using an OperatorClassification / TryGetOperatorType
2 participants
Added right click context menu to symbol library and functionality to delete a symbol.
Update: EditableSymbolProject.cs - Add/expose new MarkCodeExternallyModified to force reload/recompile from other classes
Update: SymbolAnalysis.cs - Add single-symbol analysis using TryGetSymbolInfo
Update: SymbolLibrary.cs - Add right click context for DeleteSymbolDialog
Update: SymbolUiJson.cs - Skip orphaned child UIs whose symbol child no longer exists (just deleted)