-
Notifications
You must be signed in to change notification settings - Fork 238
Respect tabs vs spaces when generating code #1606
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,9 +18,16 @@ public sealed class CodeWriter | |
| private int _currentLineIndex; | ||
| private int _currentLineCharacterIndex; | ||
|
|
||
| public CodeWriter() | ||
| public CodeWriter() : this(Environment.NewLine, 4, false) | ||
| { | ||
| NewLine = Environment.NewLine; | ||
| } | ||
|
|
||
| public CodeWriter(string newLine, int tabSize, bool indentWithTabs) | ||
| { | ||
| NewLine = newLine; | ||
| TabSize = tabSize; | ||
| IndentWithTabs = indentWithTabs; | ||
|
|
||
| _builder = new StringBuilder(); | ||
| } | ||
|
|
||
|
|
@@ -47,6 +54,10 @@ public string NewLine | |
| } | ||
| } | ||
|
|
||
| public int TabSize { get; set; } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. public setters? |
||
|
|
||
| public bool IndentWithTabs { get; set; } | ||
|
|
||
| public SourceLocation Location => new SourceLocation(_absoluteIndex, _currentLineIndex, _currentLineCharacterIndex); | ||
|
|
||
| public char this[int index] | ||
|
|
@@ -62,15 +73,40 @@ public char this[int index] | |
| } | ||
| } | ||
|
|
||
| // Internal for testing. | ||
| internal CodeWriter Indent(int size) | ||
| public CodeWriter Indent(int size) | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made this public. Otherwise things in |
||
| { | ||
| if (Length == 0 || this[Length - 1] == '\n') | ||
| { | ||
| _builder.Append(' ', size); | ||
| var actualSize = 0; | ||
| if (IndentWithTabs) | ||
| { | ||
| // Avoid writing directly to the StringBuilder here, that will throw off the manual indexing | ||
| // done by the base class. | ||
| var tabs = size / TabSize; | ||
| actualSize += tabs; | ||
| for (var i = 0; i < tabs; i++) | ||
| { | ||
| _builder.Append("\t"); | ||
| } | ||
|
|
||
| var spaces = size % TabSize; | ||
| actualSize += spaces; | ||
| for (var i = 0; i < spaces; i++) | ||
| { | ||
| _builder.Append(" "); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| actualSize = size; | ||
| for (var i = 0; i < size; i++) | ||
| { | ||
| _builder.Append(" "); | ||
| } | ||
| } | ||
|
|
||
| _currentLineCharacterIndex += size; | ||
| _absoluteIndex += size; | ||
| _currentLineCharacterIndex += actualSize; | ||
| _absoluteIndex += actualSize; | ||
| } | ||
|
|
||
| return this; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,7 +32,7 @@ public override RazorCSharpDocument WriteDocument(RazorCodeDocument codeDocument | |
| } | ||
|
|
||
| var context = new DefaultCodeRenderingContext( | ||
| new CodeWriter(), | ||
| new CodeWriter(Environment.NewLine, _options.IndentSize, _options.IndentWithTabs), | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| _codeTarget.CreateNodeWriter(), | ||
| codeDocument, | ||
| documentNode, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,10 @@ public async override Task<RazorLSPOptions> GetLatestOptionsAsync() | |
| { | ||
| Items = new[] | ||
| { | ||
| new ConfigurationItem() | ||
| { | ||
| Section = "editor" | ||
| }, | ||
| new ConfigurationItem() | ||
| { | ||
| Section = "razor" | ||
|
|
@@ -49,16 +53,22 @@ public async override Task<RazorLSPOptions> GetLatestOptionsAsync() | |
| }; | ||
|
|
||
| var result = await _server.Client.SendRequest<ConfigurationParams, object[]>("workspace/configuration", request); | ||
| if (result == null || result.Length < 1 || result[0] == null) | ||
| if (result == null || result.Length < 2 || result[0] == null) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What makes 2 the expected length, and as a corollary, why was it one before?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The spec says the result should be the same length as the number of |
||
| { | ||
| _logger.LogWarning("Client failed to provide the expected configuration."); | ||
| return null; | ||
| } | ||
|
|
||
| var jsonString = result[0].ToString(); | ||
| var builder = new ConfigurationBuilder(); | ||
| using var stream = new MemoryStream(Encoding.UTF8.GetBytes(jsonString)); | ||
| builder.AddJsonStream(stream); | ||
|
|
||
| var editorJsonString = result[0].ToString(); | ||
| using var editorStream = new MemoryStream(Encoding.UTF8.GetBytes(editorJsonString)); | ||
| builder.AddJsonStream(editorStream); | ||
|
|
||
| var razorJsonString = result[1].ToString(); | ||
| using var razorStream = new MemoryStream(Encoding.UTF8.GetBytes(razorJsonString)); | ||
| builder.AddJsonStream(razorStream); | ||
|
|
||
| var config = builder.Build(); | ||
|
|
||
| var instance = BuildOptions(config); | ||
|
|
@@ -83,7 +93,19 @@ private RazorLSPOptions BuildOptions(IConfiguration config) | |
| enableFormatting = parsedEnableFormatting; | ||
| } | ||
|
|
||
| return new RazorLSPOptions(trace, enableFormatting); | ||
| var tabSize = instance.TabSize; | ||
| if (int.TryParse(config["tabSize"], out var parsedTabSize)) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a guarantee that
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Guarantee is a strong word but we can expect vscode to do the right thing. Moreover, if it is not there it still won't throw. The default implementation of |
||
| { | ||
| tabSize = parsedTabSize; | ||
| } | ||
|
|
||
| var insertSpaces = instance.InsertSpaces; | ||
| if (bool.TryParse(config["insertSpaces"], out var parsedInsertSpaces)) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. |
||
| { | ||
| insertSpaces = parsedInsertSpaces; | ||
| } | ||
|
|
||
| return new RazorLSPOptions(trace, enableFormatting, tabSize, insertSpaces); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
| using Microsoft.AspNetCore.Razor.LanguageServer.Common; | ||
| using Microsoft.AspNetCore.Razor.LanguageServer.ProjectSystem; | ||
| using Microsoft.CodeAnalysis.Razor; | ||
| using Microsoft.Extensions.Options; | ||
|
|
||
| namespace Microsoft.AspNetCore.Razor.LanguageServer | ||
| { | ||
|
|
@@ -15,16 +16,23 @@ internal class RemoteProjectSnapshotProjectEngineFactory : DefaultProjectSnapsho | |
| public static readonly IFallbackProjectEngineFactory FallbackProjectEngineFactory = new FallbackProjectEngineFactory(); | ||
|
|
||
| private readonly FilePathNormalizer _filePathNormalizer; | ||
| private readonly IOptionsMonitor<RazorLSPOptions> _optionsMonitor; | ||
|
|
||
| public RemoteProjectSnapshotProjectEngineFactory(FilePathNormalizer filePathNormalizer) : | ||
| public RemoteProjectSnapshotProjectEngineFactory(FilePathNormalizer filePathNormalizer, IOptionsMonitor<RazorLSPOptions> optionsMonitor) : | ||
| base(FallbackProjectEngineFactory, ProjectEngineFactories.Factories) | ||
| { | ||
| if (filePathNormalizer == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(filePathNormalizer)); | ||
| } | ||
|
|
||
| if (optionsMonitor is null) | ||
| { | ||
| throw new ArgumentNullException(nameof(optionsMonitor)); | ||
| } | ||
|
|
||
| _filePathNormalizer = filePathNormalizer; | ||
| _optionsMonitor = optionsMonitor; | ||
| } | ||
|
|
||
| public override RazorProjectEngine Create( | ||
|
|
@@ -39,7 +47,37 @@ public override RazorProjectEngine Create( | |
| } | ||
|
|
||
| var remoteFileSystem = new RemoteRazorProjectFileSystem(defaultFileSystem.Root, _filePathNormalizer); | ||
| return base.Create(configuration, remoteFileSystem, configure); | ||
| return base.Create(configuration, remoteFileSystem, Configure); | ||
|
|
||
| void Configure(RazorProjectEngineBuilder builder) | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not completely sure if this is the right place to wire this up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not awful |
||
| { | ||
| configure(builder); | ||
| builder.Features.Add(new RemoteCodeGenerationOptionsFeature(_optionsMonitor)); | ||
| } | ||
| } | ||
|
|
||
| private class RemoteCodeGenerationOptionsFeature : RazorEngineFeatureBase, IConfigureRazorCodeGenerationOptionsFeature | ||
| { | ||
| private readonly IOptionsMonitor<RazorLSPOptions> _optionsMonitor; | ||
|
|
||
| public RemoteCodeGenerationOptionsFeature(IOptionsMonitor<RazorLSPOptions> optionsMonitor) | ||
| { | ||
| if (optionsMonitor is null) | ||
| { | ||
| throw new ArgumentNullException(nameof(optionsMonitor)); | ||
| } | ||
|
|
||
| _optionsMonitor = optionsMonitor; | ||
| } | ||
|
|
||
| public int Order { get; set; } | ||
|
|
||
| public void Configure(RazorCodeGenerationOptionsBuilder options) | ||
| { | ||
| // We don't need to explicitly subscribe to options changing because this method will be run on every parse. | ||
| options.IndentSize = _optionsMonitor.CurrentValue.TabSize; | ||
| options.IndentWithTabs = !_optionsMonitor.CurrentValue.InsertSpaces; | ||
| } | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.