Skip to content

Respect tabs vs spaces when generating code#1606

Closed
ajaybhargavb wants to merge 1 commit into
masterfrom
ajbaaska/respect-tabs
Closed

Respect tabs vs spaces when generating code#1606
ajaybhargavb wants to merge 1 commit into
masterfrom
ajbaaska/respect-tabs

Conversation

@ajaybhargavb
Copy link
Copy Markdown

@ajaybhargavb ajaybhargavb commented Feb 19, 2020

Fixes https://github.com/dotnet/aspnetcore/issues/18996

  • Made TabSize and IndentWithTabs first class properties on CodeWriter and actually use them when indenting.
  • Wire it up with the options provided by the client in LSP scenarios


// Internal for testing.
internal CodeWriter Indent(int size)
public CodeWriter Indent(int size)
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this public. Otherwise things in Extensions.1_X projects etc were complaining because we share CodeWriterExtensions.cs as a shared source and it uses this method.

return base.Create(configuration, remoteFileSystem, configure);
return base.Create(configuration, remoteFileSystem, Configure);

void Configure(RazorProjectEngineBuilder builder)
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not awful

#line default
#line hidden
#nullable disable
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why this happened. Haven't investigated it further yet. @NTaylorMullen any ideas off the top of your head?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh geez, no clue


var context = new DefaultCodeRenderingContext(
new CodeWriter(),
new CodeWriter(Environment.NewLine, _options.IndentSize, _options.IndentWithTabs),
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Environment.NewLine should also eventually come from options.

}
}

public int TabSize { get; set; }
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public setters?

return base.Create(configuration, remoteFileSystem, configure);
return base.Create(configuration, remoteFileSystem, Configure);

void Configure(RazorProjectEngineBuilder builder)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not awful

#line default
#line hidden
#nullable disable
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh geez, no clue

private int _currentLineCharacterIndex;

public CodeWriter()
public CodeWriter() : this(Environment.NewLine, 4, false)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public CodeWriter() : this(Environment.NewLine, 4, false)
public CodeWriter() : this(Environment.NewLine, 4, indentWithTabs: false)


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)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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 ConfigurationItems we pass in. I'll leave a comment for future us.


return new RazorLSPOptions(trace, enableFormatting);
var tabSize = instance.TabSize;
if (int.TryParse(config["tabSize"], out var parsedTabSize))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a guarantee that config will always have a tabSize?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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 IConfiguration will just return null.

}

var insertSpaces = instance.InsertSpaces;
if (bool.TryParse(config["insertSpaces"], out var parsedInsertSpaces))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

@JunTaoLuo
Copy link
Copy Markdown

FYI some of this PR will need to be opened against aspnetcore. Some of the projects have migrated there.

@ajaybhargavb
Copy link
Copy Markdown
Author

Closing as we're not ready to tackle this yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow Razor parser to utilize C# formatting options

4 participants