Allow Razor generated documents to respect tabs/spaces settings#31211
Conversation
allisonchou
requested review from
a team,
NTaylorMullen,
TanayParikh,
ryanbrandenburg and
sharwell
| #line hidden | ||
| #nullable disable | ||
|
|
There was a problem hiding this comment.
@TanayParikh It looks like Ajay was seeing the same issue in dotnet/razor#1606. It doesn't look too impactful, but just want to check this change is OK?
There was a problem hiding this comment.
Hmmm, so this wont have any actual impact on the runnability of the code since it's outside of the line pragma; however, it's concerning that this is being touched at all? Definitely need to look into what's happening here. Also, in the other files I wouldn't imagine that generated locations would change unless there was a mashup of tabs in the source files / interesting settings in the tests.
There was a problem hiding this comment.
Also, @pranavkm does this not worry you?
| public CodeWriter Indent(int size) | ||
| { | ||
| if (Length == 0 || this[Length - 1] == '\n') | ||
| if (size == 0 || (Length != 0 && this[Length - 1] != '\n')) |
There was a problem hiding this comment.
This is unrelated to your PR, but indexing in to StringBuilder (which is what this[..] is doing) is really slow.
There was a problem hiding this comment.
Hmm, interesting. Maybe we can keep track of the last character somehow, possibly storing it in a field?
(Don't know if it's in the scope of this PR, but would be a good follow up later on.)
…on/CodeWriter.cs Co-authored-by: Pranav K <prkrishn@hotmail.com>
NTaylorMullen
left a comment
NTaylorMullen
left a comment
There was a problem hiding this comment.
Looking good! One big concern is that the generated files changed. Need to validate that it's expected and that we were doing the wrong thing previously and not doing the wrong thing now. I wouldn't have imagined they'd change is all
|
|
||
| var context = new DefaultCodeRenderingContext( | ||
| new CodeWriter(), | ||
| new CodeWriter(Environment.NewLine, _options.IndentWithTabs, _options.IndentSize), |
There was a problem hiding this comment.
Hmmm I wonder if it makes more sense for the code writer to take in the code gen options instead. @pranavkm thoughts now that you own this? 😄
There was a problem hiding this comment.
I think you're right, it would also make it easier in the future if we were to add other code gen option variables to the constructor.
| #line hidden | ||
| #nullable disable | ||
|
|
There was a problem hiding this comment.
Hmmm, so this wont have any actual impact on the runnability of the code since it's outside of the line pragma; however, it's concerning that this is being touched at all? Definitely need to look into what's happening here. Also, in the other files I wouldn't imagine that generated locations would change unless there was a mashup of tabs in the source files / interesting settings in the tests.
|
Okay, so I think I see what's going on here in regards to why the code gen and mappings have changed. In the existing code, the indent logic for padding is done here in CodeWriterExtensions. However, I believe this logic is incorrect since it does not properly increment the |
…b.com/dotnet/aspnetcore into dev/allichou/TabsSpacesCodeGeneration
Ahhhh I can totally see that. Thank you for validating @allisonchou ! |
|
The failure looks random, will probably pass on retry. |
Or, you could file an issue and mark the test as quarantined. |
|
#31405 |
8 participants
Part of dotnet/razor#3317. Enables generated documents to respect the tabs/spaces settings passed in from Razor tooling.
The bulk of this is Ajay's work from dotnet/razor#1606, so major creds to him. 🎉