Allow generate ctor intent to specify accessibility#59730
Allow generate ctor intent to specify accessibility#59730JoeRobich merged 9 commits intodotnet:release/dev17.2from
Conversation
|
cc @pgroene |
| }"; | ||
|
|
||
| // lang=json | ||
| var intentData = @"{ ""Accessibility"": ""Private""}"; |
There was a problem hiding this comment.
done - note that the values correspond to https://sourceroslyn.io/#Microsoft.CodeAnalysis/Symbols/Accessibility.cs,12
src/Workspaces/Core/Portable/Shared/Extensions/SyntaxGeneratorExtensions.cs
Outdated
Show resolved
Hide resolved
| Accessibility? desiredAccessibility = null; | ||
| if (serializedIntentData != null) | ||
| { | ||
| var intentData = JsonSerializer.Deserialize<GenerateConstructorIntentData>(serializedIntentData, _jsonSerializerOptions); |
There was a problem hiding this comment.
It feels like a leaky abstraction to have this know about json deserialization etc. Is it overkill to have an IntentDeserializer service that hides the json bits? Or can this be moved up to a higher level (I admit I haven't looked to see where serializedIntentData comes from)
There was a problem hiding this comment.
I think this is better - lmk!
b3f6df2 to
20112b9
Compare
| }"; | ||
|
|
||
| // lang=json | ||
| var intentData = @"{ ""accessibility"": ""private""}"; |
There was a problem hiding this comment.
by the way syntax highlighting for this is great, thanks @CyrusNajmabadi
Now I just need raw strings to get rid of those extra "
| { | ||
| PropertyNamingPolicy = JsonNamingPolicy.CamelCase, | ||
| }; | ||
| _serializerOptions.Converters.Add(new JsonStringEnumConverter(JsonNamingPolicy.CamelCase)); |
There was a problem hiding this comment.
one concern i have is how are json exceptions handled when you use this? i think we need to presume the data may not be well formed, and we should be resilient to that.
There was a problem hiding this comment.
Done - added a report and catch to the deserialization path so that we know when it goes wrong. Since intellicode is providing the json, we definitely want to be notified if it is malformed.
...erateConstructorFromMembers/AbstractGenerateConstructorFromMembersCodeRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Shared/Extensions/SyntaxGeneratorExtensions.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Shared/Extensions/SyntaxGeneratorExtensions.cs
Outdated
Show resolved
Hide resolved
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
tentative approval with review feedback handled.
|
Is this going through QB @dibarbet? |
|
@RikkiGibson it was supposed to be m2 but the integration test failures blocked it for a while. |
Fixes issue where the generation would overwrite the typed accessibility.
Commit at a time recommended - the first commit just nullable enables a file.