Skip to content

[WIP] Add Debugger display attribute #402

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

Closed
wants to merge 7 commits into from

Conversation

jwood803
Copy link
Contributor

Proposed fix for #194.

Updated the CSharpAPIGenerator file to include the DebuggerDisplay attribute.

Marked as WIP since I'm sure this would need to be improved.

@TomFinley
Copy link
Contributor

Hi @jwood803 , I'm not sure this will work since this is using the short names, rather than the actual field names of the properties. Also the criteria of 10 as a magic threshold on the sort value seems somewhat arbitrary.

@@ -214,6 +214,12 @@ private void GenerateClasses(IndentingTextWriter writer, Type inputType, ModuleC
classBase = $" : OneToOneColumn<{apiName}>, IOneToOneColumn";
else if (type.IsSubclassOf(typeof(ManyToOneColumn)))
classBase = $" : ManyToOneColumn<{apiName}>, IManyToOneColumn";

if (inputAttr.ShortName != null && inputAttr.SortOrder < 10)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sure this isn't the right way to check if the attribute should be included or not. Would need some guidance on if the ShortName is the best one to use and how best to use the SortOrder.

Copy link
Contributor

Choose a reason for hiding this comment

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

The debugger display attribute's format strings would correspond to the property name exactly, since otherwise the debugger won't have any idea what to make of it. Those properties, as we see, are generated on this line, so if we use them we'll have to use roughly the same code.

writer.Write($"public {inputTypeString} {CSharpGeneratorUtils.Capitalize(inputAttr.Name ?? fieldInfo.Name)} {{ get; set; }}");

As you can see, the short name is not used here at all.

Also: my original suggestion in #194 is to only include in the display formatted string those parameters with the SortOrder property set (so not an arbitrary threshold, just whether it had been set at all or not -- elsewhere we use this sort order property ) and of course only those types that would display well... which is to say, probably simple types like strings, ints, floats, doubles, etc. Not sure how well arrays display in this scheme.

To be fair, I'm not actually entirely clear how this code works -- it looks like we're in the middle of an iteration over the field, but then we're in the middle of this loop writing out the class? I think perhaps @codemzs understands this code a bit better and can advise more exactly.

One thing to keep in mind is: once #371 is realized, we will no longer be generating C# code to generate JSON then pass it through reflection base mechanisms to call, again, C# code, we will instead just call the underlying C# code directly. When that happens this generator will go away.


In reply to: 197821103 [](ancestors = 197821103)

@jwood803
Copy link
Contributor Author

Thanks for the feedback, @TomFinley!

The Name property looks like it's always null. Would Aliases be the best to use here instead?

And it seems I didn't know how to use the review system. I had a comment that I thought was public but I guess it isn't until I hit the "Review" button. Apologies about that. 😛

@@ -214,6 +214,12 @@ private void GenerateClasses(IndentingTextWriter writer, Type inputType, ModuleC
classBase = $" : OneToOneColumn<{apiName}>, IOneToOneColumn";
else if (type.IsSubclassOf(typeof(ManyToOneColumn)))
classBase = $" : ManyToOneColumn<{apiName}>, IManyToOneColumn";

if(inputAttr.SortOrder > 0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is better @TomFinley, but just let me konw if I'm going down the wrong path.

@Zruty0
Copy link
Contributor

Zruty0 commented Oct 5, 2018

@jwood803 , I will be closing this PR, since CSharpApi is on the deprecation path right now.

@Zruty0 Zruty0 closed this Oct 5, 2018
@jwood803 jwood803 deleted the add-debugger-display branch October 6, 2018 11:46
@ghost ghost locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants