[release/8.0-preview7] Share SourceWriter between JSON & config binding generators #89196
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
carlossanlop
added
the
Servicing-consider
carlossanlop
approved these changes
Jul 19, 2023
ericstj
added
Servicing-approved
|
Merged as this was passing modulo one unrelated test issue. (linked) |
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport of #89150 to release/8.0-preview7
/cc @layomia
Customer Impact
Fixes an issue with the configuration binding generator where it parses multiline source strings to be formatted into C# source code. It wasn't parsing new lines correctly, leading to a compilation error in both when building the repo for development in some build configurations, and also in user applications. The issues were reported by @tannergooding and multiple community members. Fixing this would unblock these important scenarios.
The source is written with an internal custom type called
SourceWriter. The configuration & JSON generators use separate but similar implementations. The JSON implementation handles line endings correctly; so the fix here is the move the JSON copy into a common location to be used by both generators, and delete the config binding copy.There are no changes to the JSON implementation; it calls the writer APIs in the exact same way. The config implementation had to make a non-trivial diff w.r.t. lines of code changed, but those were mostly to change the names of the methods called to match the writer type.
Testing
The config binder generator has all unit tests still passing, and there's no diff to the baseline formatting tests except for a few new lines added to match the formatting of the strings in source code. This is because the JSON source writer correctly includes the new lines specified by the input source string, into the resulting formatted code. The new lines are included for better readability.
Unit test cases were added for the
SourceWritermethod that was causing the build errors. It ensures that it can handle various line endings correctly, for both consistent and mixed line-ending inputs.Risk
The JSON generator didn't change, so risk is low. The config binding generator has no test regressions, and the added
SourceWritertests should help minimize the risk here.