Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change the ObjectToTypeName value converter to give prettier C# names. #400

Merged
merged 9 commits into from Mar 26, 2019

Conversation

@dfkeenan
Copy link
Contributor

dfkeenan commented Mar 3, 2019

PR Details

This PR changes the ObjectToTypeName value converter to convert object Type to a prettier C# like style.

Description

This change makes the ObjectToTypeName value converter produce more meaningful names. The Type name returned has the following properties:

  • Generic type parameters are included using C# like syntax. i.e. <Type1, Type2> instead of '2.
  • Primitive types use the keyword syntax. i.e. int instead of Int32.
  • Nullable types display using C# nullable syntax. i.e. DateTime? instead of Nullable<DateTime>.
  • ValueTuple types display using C# tuple syntax i.e. (int, string) instead of ValueTuple<int, string>.
  • Type names are not namespace or class name qualified. i.e. DateTime instead of System.DateTime.

Related Issue

None

Motivation and Context

Currently when displaying a types name in the property grid it does not show meaningful information for generic types. For example a property of type MyCustomClass<int?> gets displayed as MyCustomClass'1. This is not very helpful. The goal of this change is to make it return more meaningfull names such as MyCustomClass<int?>.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
@dfkeenan

This comment has been minimized.

Copy link
Contributor Author

dfkeenan commented Mar 3, 2019

I currently have a few outstanding issues.

  • For some reason unit tests don't run. I get messages like Test project Xenko.Core.Presentation.Tests does not reference any .NET NuGet adapter. Test discovery or execution might not work for this project.. So I have not run existing tests or created new ones.
  • I am not sure if the converter should keep a cached version of type names, For example keep a static ConcurrentDictionary<Type, string> and a static CSharpCodeProvider.
@dfkeenan

This comment has been minimized.

Copy link
Contributor Author

dfkeenan commented Mar 4, 2019

@xen2 , Do we need to install the xunit.runner.visualstudio NuGet package in the test projects now? See https://xunit.github.io/docs/getting-started/netfx/visual-studio#run-tests-visualstudio.

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Mar 4, 2019

@dfkeenan
This is supposedly already added, but might be missing from some test projects?

samples/Tests/Xenko.Samples.Tests.csproj:    <PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" />
sources/assets/Xenko.Core.Assets.Quantum.Tests/Xenko.Core.Assets.Quantum.Tests.csproj:    <PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" />
sources/assets/Xenko.Core.Assets.Tests/Xenko.Core.Assets.Tests.csproj:    <PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" />
sources/buildengine/Xenko.Core.BuildEngine.Tests/Xenko.Core.BuildEngine.Tests.csproj:    <PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" />
sources/core/Xenko.Core.Design.Tests/Xenko.Core.Design.Tests.csproj:    <PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" />
sources/core/Xenko.Core.Mathematics.Tests/Xenko.Core.Mathematics.Tests.csproj:    <PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" />
sources/core/Xenko.Core.Tests/Xenko.Core.Tests.Android.csproj:    <PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" />
sources/core/Xenko.Core.Tests/Xenko.Core.Tests.csproj:    <PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" />
sources/core/Xenko.Core.Tests/Xenko.Core.Tests.iOS.csproj:    <PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" />
sources/core/Xenko.Core.Yaml.Tests/Xenko.Core.Yaml.Tests.csproj:    <PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" />
sources/editor/Xenko.Core.Assets.Editor.Tests/Xenko.Core.Assets.Editor.Tests.csproj:    <PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" />
sources/editor/Xenko.GameStudio.Tests/Xenko.GameStudio.Tests.csproj:    <PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" />
sources/engine/Xenko.Assets.Tests/Xenko.Assets.Tests.csproj:    <PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" />
sources/engine/Xenko.Graphics.Regression/Xenko.Graphics.Regression.csproj:    <PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" />
sources/engine/Xenko.Shaders.Tests/Xenko.Shaders.Tests.Windows.csproj:    <PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" />
sources/presentation/Xenko.Core.Presentation.Quantum.Tests/Xenko.Core.Presentation.Quantum.Tests.csproj:    <PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" />
sources/presentation/Xenko.Core.Quantum.Tests/Xenko.Core.Quantum.Tests.csproj:    <PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" />
sources/tools/Xenko.Code.Tests/Xenko.Code.Tests.csproj:    <PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" />
sources/tools/Xenko.Core.ProjectTemplating.Tests/Xenko.Core.ProjectTemplating.Tests.csproj:    <PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" />
@dfkeenan

This comment has been minimized.

Copy link
Contributor Author

dfkeenan commented Mar 4, 2019

Looks like it is missing in the one test project I was looking at Xenko.Core.Presentation.Tests. I will add it to that.

@dfkeenan

This comment has been minimized.

Copy link
Contributor Author

dfkeenan commented Mar 9, 2019

I have added unit tests that cover a fair few cases. I have checked the All new and existing tests passed. criteria. But I only ran the tests for the assembly I changed in the test project Xenko.Core.Presentation.Tests.

@xen2 xen2 marked this pull request as ready for review Mar 9, 2019
@dfkeenan dfkeenan mentioned this pull request Mar 16, 2019
3 of 7 tasks complete
@dfkeenan

This comment has been minimized.

Copy link
Contributor Author

dfkeenan commented Mar 18, 2019

@xen2 , Would you prefer a commit per review issue or is 1 "Resolve review issues" commit OK?

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Mar 18, 2019

@xen2 , Would you prefer a commit per review issue or is 1 "Resolve review issues" commit OK?

I will probably do a squash before merge so you don't need to bother.

@dfkeenan

This comment has been minimized.

Copy link
Contributor Author

dfkeenan commented Mar 18, 2019

I will probably do a squash before merge so you don't need to bother.

OK!

dfkeenan added 2 commits Mar 21, 2019
Move TypeNameHelper to own file, rename to TypeExtensions and make internal.
Tidy up ObjectToTypeName value converter.
@dfkeenan

This comment has been minimized.

Copy link
Contributor Author

dfkeenan commented Mar 21, 2019

I think I have addressed all the review issues. I wasn't sure who is supposed to "Resolve conversation" so I haven't for the time being.

Copy link
Member

xen2 left a comment

Thanks, will merge once those two small details are fixed!

@dfkeenan

This comment has been minimized.

Copy link
Contributor Author

dfkeenan commented Mar 26, 2019

OK, I have done those 2 things.

@xen2 xen2 merged commit 89e92e4 into xenko3d:master Mar 26, 2019
2 checks passed
2 checks passed
WIP ready for review
Details
license/cla Contributor License Agreement is signed.
Details
@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Mar 26, 2019

Merged, thanks!

@dfkeenan dfkeenan deleted the dfkeenan:ObjectToTypeName branch Mar 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.