Repository navigation
fix: Convert newline chars in enumDescriptions to spaces - #1013
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the CodeGenerator to normalize newlines in enumDescriptions to spaces, ensuring proper formatting for XML documentation, and exposes the NormalizeDescriptions method as internal to allow for unit testing. A review comment suggests using Children().OfType<JValue>() instead of Children<JValue>() to avoid potential InvalidCastException errors if non-value elements are present in the enumDescriptions array.
|
|
||
| namespace Google.Api.Generator.Rest.Tests | ||
| { | ||
| public class DescriptionNewlineTest |
There was a problem hiding this comment.
Instead of adding these, let's add Compute to golden tests.
There was a problem hiding this comment.
Sounds good. For golden tests do we add the discovery doc and *.g.cs as-is or do we trim it down? compute.preview looks to be considerably larger than our current golden tests, about 15MB for the discovery doc and generated source file which is larger that the rest of the golden test files combined.
edit: I've added the whole thing for now
| } | ||
|
|
||
| private static string NormalizeDescriptions(string discoveryJson) | ||
| internal static string NormalizeDescriptions(string discoveryJson) |
There was a problem hiding this comment.
If we add compute to golden tests we don't need to make this internal.
|
|
||
| // Enum descriptions are used for XML documentation as well (for enums generated for parameters), | ||
| // so they need the same normalization. | ||
| var enumDescriptions = raw.Descendants() |
There was a problem hiding this comment.
Let's concat descriptions and enumDescriptions to keep a single foreach loop.
There was a problem hiding this comment.
Updated to concat the two
4bc17fa to
375f53a
Compare
375f53a to
b43fcb8
Compare
amanda-tarafa
left a comment
amanda-tarafa
left a comment
There was a problem hiding this comment.
LGTM! Thanks
And yes, it's OK to add Compute whole. If something complains at some point because o the size, we'll deal with it.
|
@amanda-tarafa Do you know what requirement need to be met for the gapic-generator-csharp-presubmit? |
That's broken, we should remove it. But it's optional, you can ignore it and merge. |
b/567102951