Skip to content
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

CodeFormatter should accept SyntaxTree #621

Closed
belav opened this issue Feb 22, 2022 · 8 comments · Fixed by #625
Closed

CodeFormatter should accept SyntaxTree #621

belav opened this issue Feb 22, 2022 · 8 comments · Fixed by #625
Milestone

Comments

@belav
Copy link
Owner

belav commented Feb 22, 2022

There are cases where someone has a SourceText and being forced to convert it to a string is an extra step that shouldn't be required. CodeFormatter could pretty easily accept SourceText as well.

@TwentyFourMinutes
Copy link

I actually noticed that a SourceText is already a finalized visual form of the SyntaxTree, so it therefore really should instead have an overload accepting the actual SyntaxTree, what do you think?

@belav
Copy link
Owner Author

belav commented Feb 26, 2022

Yeah that makes sense. Maybe it has an overload for both of them?

@TwentyFourMinutes
Copy link

I don't see any obvious way to convert a SourceText to a SyntaxTree without converting it to a string and back. Therefore the consumer should really be aware what's happening IMHO.

@belav
Copy link
Owner Author

belav commented Feb 27, 2022

CSharpSyntaxTree.ParseText has an overload that accepts a SourceText that you may have missed.

@TwentyFourMinutes
Copy link

Oh I think that we are both talking about different things. When I am creating a source code through the SyntaxFactory API I am currently first converting the SyntaxTree > SourceText > string. Now from my perspective it would make the most sense if I could directly pass the CodeFormatter the SyntaxTree. Not because it is necessarily less code to write, but rather because it is a lot faster, as you are converting the passed string/SourceText back to a SyntaxTree as far as I am aware of.

@belav
Copy link
Owner Author

belav commented Feb 28, 2022

Yeah, I was thinking CodeFormatter would have a few overloads

public static string Format(string code, CodeFormatterOptions? options = null)

public static string Format(SourceText code, CodeFormatterOptions? options = null)

public static string Format(SyntaxTree code, CodeFormatterOptions? options = null)

I assumed you were looking at how to implement those overloads in csharpier and didn't know about that CSharpSyntaxTree.ParseText had an overload for SourceText

@TwentyFourMinutes
Copy link

Oh I see 😄. Well that would make sense however do you think that it should be encourage to pass it a SourceText? CSharpSyntaxTree.ParseText will more or less convert it to a string anyway and the performance hit would just be hidden. Also I can't think of any quick scenario in my head where I'd have the SourceText, but not the SyntaxTree representation of a piece of code.

@belav belav added this to the 0.16.0 milestone Feb 28, 2022
@belav belav changed the title CodeFormatter should accept SourceText CodeFormatter should accept SyntaxTree Feb 28, 2022
belav added a commit that referenced this issue Feb 28, 2022
@belav
Copy link
Owner Author

belav commented Feb 28, 2022

I am good with just having the overload for SyntaxTree, no need to add another overload if no one is going to need it.

shocklateboy92 added a commit that referenced this issue Mar 2, 2022
closes #621

Co-authored-by: Lasath Fernando <devel@lasath.org>
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 a pull request may close this issue.

2 participants