unoplatform / Uno.Core

Uno.Core is a set of helpers and extension methods used to accelerate development.
Other
39 stars 12 forks source link

Append line terminator in IndentedStringBuilder.AppendLine (#55) #58

Closed gahag closed 5 years ago

gahag commented 5 years ago

Corresponding to the behavior of .net's StringBuilder, the method AppendLine should append a line terminator at the end.

GitHub Issue (If applicable): #55

PR Type

What kind of change does this PR introduce?

What is the current behavior?

AppendLine(String) does not insert a new line at the end.

What is the new behavior?

AppendLine(String) does inserts a new line at the end.

PR Checklist

Please check if your PR fulfills the following requirements:

I believe this can be a breaking change, but I'm not sure how should we proceed regarding this matter. Also, I've not added a test for this method as I haven't found a unit test for this class. Should we work to add these tests in this pull request?

gahag commented 5 years ago

@carldebilly do you know how can I get a reviewer for this pull request? Thanks.

davidjohnoliver commented 5 years ago

Hi @gahag !

Thanks for your contribution, but I don't think we'll be able to merge this change. This method is used extensively for code generation in the Uno.UI repo and this will be a big breaking change there. Probably this method was misnamed originally, but we're stuck with it now.

If you're still willing to contribute, it would be great to add xml comments documenting how the method actually behaves and the correct usage if you want an actual line break (which is to call AppendLine() with no parameters).

gahag commented 5 years ago

This seems reasonable. I will make another pr adding the documentation. Thanks for the review!