Skip to content

Bug fix issue 30351 #46436

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.

Sign up for GitHub

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

Jump to bottom
Merged
buyaa-n merged 10 commits into from
Feb 3, 2021
Merged

Bug fix issue 30351 #46436

buyaa-n merged 10 commits into from
Feb 3, 2021

Conversation

@NewellClark
Copy link
Contributor

@NewellClark NewellClark commented Dec 28, 2020

This fixes issue 30351. I noticed there weren't any unit tests for System.CodeDom.IndentedTextWriter, so I went ahead and added those as well.

@NewellClark
Added tests for IndentedTextWriter
5f14083 
Added failing unit tests that demonstrate Issue dotnet#30351 with the IndentedTextWriter.
@NewellClark
IndentedTextWriter async overrides
f55d1d7 
Added overrides for async methods on IndentedTextWriter that call the respective async methods on the underlying TextWriter. 

Fix dotnet#30351
@ghost
Copy link

ghost commented Dec 28, 2020

Tagging subscribers to this area: @buyaa-n, @krwq
See info in area-owners.md if you want to be subscribed.

Issue Details

This fixes issue 30351. I noticed there weren't any unit tests for System.CodeDom.IndentedTextWriter, so I went ahead and added those as well.

Author: NewellClark
Assignees: -
Labels:

area-System.CodeDom
Milestone: -

@NewellClark
Undid api modification in IndentedTextWriter
df3bed0 
I can fix issue dotnet#30351 without modifying the API of IndentedTextWriter. 

Fix dotnet#30351
@NewellClark
Fix globalization issue with unit test
aa24d7f 
Apparently some of the CI tests run on a machine in a locale where commas are used as the decimal point. This caused some unit tests to fail. I've  changed those tests to use the invariant culture.

Fix issue dotnet#30351
buyaa-n
buyaa-n reviewed Dec 30, 2020
View reviewed changes
@NewellClark
Made suggested edits
612577d 
Corrected errors in refs.
Added a unit test for parameterless WriteLineAsync method. 
Cleaned up unit test with pointless interpolated string.
@NewellClark NewellClark reopened this Dec 30, 2020
@azure-pipelines
Copy link

azure-pipelines bot commented Dec 30, 2020

Commenter does not have sufficient privileges for PR 46436 in repo dotnet/runtime

buyaa-n
buyaa-n reviewed Dec 30, 2020
View reviewed changes
@NewellClark
Reverted unintended changes to System.Runtime.sln
94f991c 
VS made useless changes to System.Runtime.sln file.
@NewellClark NewellClark reopened this Dec 30, 2020
@buyaa-n
Copy link
Contributor

buyaa-n commented Dec 30, 2020

Thank you @NewellClark, the PR looks good to me, i have 2 questions for @stephentoub and @carlossanlop

  1. Do we want to add the APIs into ref? i don't see any reason not to except it is not gone through API review
  2. I am not sure if the tests should have added in System.IO, i am OK having it in CodeDom though, just saw there is existing test for IndentedTextWriter in System.IO

@NewellClark
Copy link
Contributor Author

NewellClark commented Dec 30, 2020 •
edited
Loading

Thank you @NewellClark, the PR looks good to me, i have 2 questions for @stephentoub and @carlossanlop

  1. Do we want to add the APIs into ref? i don't see any reason not to except it is not gone through API review
  2. I am not sure if the tests should have added in System.IO, i am OK having it in CodeDom though, just saw there is existing test for IndentedTextWriter in System.IO

I have just a few comments:

  1. While we're discussing APIs, we may want to consider adding protected virtual Task OutputTabsAsync(), as well as public Task WriteLineNoTabsAsync(string? s), to match the existing synchronous APIs that are specific to IndentedTextWriter. For example, if someone subclasses and overrides OutputTabs(), they should really also be overriding OutputTabsAsync(), otherwise they'll get the same bug in their subclass that we just fixed in IndentedTextWriter.
    I can submit an API review if you'd like.
  2. Oops, My bad. I didn't see the existing tests in System.IO.

@buyaa-n
Copy link
Contributor

buyaa-n commented Jan 6, 2021

I can submit an API review if you'd like.

Sorry for late response @NewellClark, I was in doubt if its need to go through API review, in general all APIs added to ref should, but it is just adding missing overloads not adding anything new. I talk with @stephentoub offline and confirmed we don't need API review here

While we're discussing APIs, we may want to consider adding protected virtual Task OutputTabsAsync(), as well as public Task WriteLineNoTabsAsync(string? s), to match the existing synchronous APIs that are specific to IndentedTextWrite. For example, if someone subclasses and overrides OutputTabs(), they should really also be overriding OutputTabsAsync(), otherwise they'll get the same bug in their subclass that we just fixed in IndentedTextWriter.

That is good point, those are also looks like missing implementation, as synchronous versions already in the ref

runtime/src/libraries/System.Runtime/ref/System.Runtime.cs

Line 5160 in 167c598

protected virtual void OutputTabs() { }
I agree we should add Async versions too

Oops, My bad. I didn't see the existing tests in System.IO.

Yeah it is confusing, from namespace i would think it is related to CodeDome, i guess it is added to IO because it is a writer, we might want to move tests to IO @carlossanlop what you think?

@NewellClark
Copy link
Contributor Author

NewellClark commented Jan 7, 2021

@buyaa-n Sounds good. I'll move the tests to System.IO and add the async versions of existing methods previously discussed.

@NewellClark
Added async versions of existing APIs
32d664d 
- Added OutputTabsAsync()
- Added WriteLineNoTabsAsync(string)
buyaa-n
buyaa-n approved these changes Jan 11, 2021
View reviewed changes
Copy link
Contributor

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

Thank you very much @NewellClark, looks good to me, lets wait for System.IO area owner @carlossanlop's review/approval

carlossanlop
carlossanlop reviewed Jan 15, 2021
View reviewed changes
carlossanlop
carlossanlop suggested changes Jan 15, 2021
View reviewed changes
Copy link
Contributor

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

Looks good. I left a few suggestions for you to consider. Please add documentation so it can be approved.

@NewellClark
Copy link
Contributor Author

NewellClark commented Jan 23, 2021

@carlossanlop I've implemented your suggested changes.

@buyaa-n
Copy link
Contributor

buyaa-n commented Feb 3, 2021

Thank you @NewellClark, we can merge it now

@buyaa-n buyaa-n merged commit 13296cb into dotnet:master Feb 3, 2021
@NewellClark NewellClark deleted the bug-fix-issue-30351 branch February 3, 2021 18:44
@ghost ghost locked as resolved and limited conversation to collaborators Mar 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoub stephentoub Awaiting requested review from stephentoub

2 more reviewers

@buyaa-n buyaa-n buyaa-n approved these changes

@carlossanlop carlossanlop carlossanlop approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@carlossanlop carlossanlop

Labels

area-System.CodeDom area-System.IO

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

IndentedTextWriter Async methods don't indent properly

4 participants

@NewellClark @buyaa-n @carlossanlop @Dotnet-GitSync-Bot