Skip to content

User Story 1565289: [MIEngine Natvis] Support multiple natvis files in visualizerFile launch configuration#1381

Merged
gc46 merged 22 commits into
mainfrom
dev/gabchen/user-story-1565289
Feb 3, 2023
Merged

User Story 1565289: [MIEngine Natvis] Support multiple natvis files in visualizerFile launch configuration#1381
gc46 merged 22 commits into
mainfrom
dev/gabchen/user-story-1565289

Conversation

@gc46

@gc46 gc46 commented Jan 24, 2023

Copy link
Copy Markdown
Contributor

No description provided.

@gc46 gc46 requested a review from WardenGnaw January 24, 2023 22:22

@WardenGnaw WardenGnaw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread test/CppTests/OpenDebug/CrossPlatCpp/LaunchCommand.cs Outdated
Comment thread test/CppTests/OpenDebug/CrossPlatCpp/LaunchCommand.cs Outdated
Comment thread test/CppTests/CppTests.csproj Outdated
Comment thread test/CppTests/Tests/NatvisTests.cs
Comment thread src/MICore/LaunchOptions.cs Outdated
@gregg-miskelly

Copy link
Copy Markdown
Member
        }

I think you want to remove the set and backing field and just have:

        /// <summary>
        /// Collection of natvis files to use when evaluating
        /// </summary>
        public List<string> VisualizerFile { get; } = new List<string>();

Refers to: src/MICore/LaunchOptions.cs:974 in 50400ad. [](commit_id = 50400ad, deletion_comment = False)

Comment thread src/MICore/LaunchOptions.xsd.types.designer.cs Outdated
Comment thread src/MICore/JsonLaunchOptions.cs Outdated
Comment thread src/MICore/JsonLaunchOptions.cs Outdated
Comment thread src/MIDebugEngine/Natvis.Impl/Natvis.cs Outdated
Comment thread test/CppTests/OpenDebug/CrossPlatCpp/LaunchCommand.cs Outdated
@gc46

gc46 commented Jan 26, 2023

Copy link
Copy Markdown
Contributor Author
        }

I think you want to remove the set and backing field and just have:

        /// <summary>
        /// Collection of natvis files to use when evaluating
        /// </summary>
        public List<string> VisualizerFile { get; } = new List<string>();

Refers to: src/MICore/LaunchOptions.cs:974 in 50400ad. [](commit_id = 50400ad, deletion_comment = False)

Why do we not want the set()?

@gregg-miskelly

Copy link
Copy Markdown
Member

Why do we not want the set()?

Generally, properties with a collection type shouldn't have a set method. Instead, they get assigned in the constructor and items are added to them.

Comment thread test/CppTests/Tests/NatvisTests.cs Outdated
Comment thread test/CppTests/Tests/NatvisTests.cs
Comment thread src/MICore/LaunchOptions.xsd.types.designer.cs
@gregg-miskelly

Copy link
Copy Markdown
Member
        }

This is still active.

If it makes your life easier, you can allow a set method, but in this case I would make these other changes:

  • Still initialize the backing field to an empty collection so you don't need those null checks.
  • Only allow set if the backing field is still an empty collection

In reply to: 1404353242


Refers to: src/MICore/LaunchOptions.cs:974 in 50400ad. [](commit_id = 50400ad, deletion_comment = False)

@gregg-miskelly

Copy link
Copy Markdown
Member
    /// Don't change this test without checking with C++ team.

We are changing this test.


Refers to: src/MICoreUnitTests/BasicLaunchOptionsTests.cs:255 in 3cb7931. [](commit_id = 3cb7931, deletion_comment = False)

Comment thread src/MICoreUnitTests/BasicLaunchOptionsTests.cs Outdated
Comment thread src/MIDebugEngine/Natvis.Impl/Natvis.cs Outdated
Comment thread src/MIDebugEngine/Natvis.Impl/Natvis.cs Outdated
Comment thread src/MIDebugEngine/Natvis.Impl/Natvis.cs
Comment thread test/CppTests/OpenDebug/CrossPlatCpp/LaunchCommand.cs Outdated
@gc46

gc46 commented Feb 1, 2023

Copy link
Copy Markdown
Contributor Author
        }

This is still active.

If it makes your life easier, you can allow a set method, but in this case I would make these other changes:

  • Still initialize the backing field to an empty collection so you don't need those null checks.
  • Only allow set if the backing field is still an empty collection

In reply to: 1404353242

Refers to: src/MICore/LaunchOptions.cs:974 in 50400ad. [](commit_id = 50400ad, deletion_comment = False)

The reason I allowed for set is due to the VisualizerFiles assignment in other places throughout the codebase such as the one shown below:
image

We can disallow the ability to set VisualizerFiles though if you deem it fine.

@WardenGnaw

Copy link
Copy Markdown
Member
        }

This is still active.
If it makes your life easier, you can allow a set method, but in this case I would make these other changes:

  • Still initialize the backing field to an empty collection so you don't need those null checks.
  • Only allow set if the backing field is still an empty collection

In reply to: 1404353242
Refers to: src/MICore/LaunchOptions.cs:974 in 50400ad. [](commit_id = 50400ad, deletion_comment = False)

The reason I allowed for set is due to the VisualizerFiles assignment in other places throughout the codebase such as the one shown below: image

We can disallow the ability to set VisualizerFiles though if you deem it fine.

You can let LaunchOptions.VisualizerFiles be created during the constructor and it will be empty.
When we need to assign/pass the Users strings you can use this.VisualizerFiles.AddRange(option.VisualizerFiles).

Comment thread src/MICore/JsonLaunchOptions.cs
Comment thread src/MICore/LaunchOptions.cs Outdated
Comment thread src/MICore/LaunchOptions.cs Outdated
Comment thread src/MICore/LaunchOptions.cs Outdated
Comment thread src/MIDebugEngine/Natvis.Impl/Natvis.cs
Comment thread test/CppTests/OpenDebug/CrossPlatCpp/LaunchCommand.cs Outdated
Comment thread src/MICore/JsonLaunchOptions.cs Outdated
Comment thread src/MICore/JsonLaunchOptions.cs Outdated
Comment thread src/MICore/MICoreResources.resx Outdated
@gc46 gc46 merged commit 153a9e5 into main Feb 3, 2023
@gc46 gc46 deleted the dev/gabchen/user-story-1565289 branch February 3, 2023 04:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants