Cleanup XmlReader by kronic · Pull Request #51561 · dotnet/runtime

kronic · 2021-04-20T13:24:14Z

Cleanup XmlReader{Async}

ghost · 2021-04-20T13:24:20Z

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

Issue Details

Cleanup XmlReader{Async}

Author:	kronic
Assignees:	-
Labels:	`area-System.Xml`
Milestone:	-

krwq · 2021-04-20T15:16:59Z

-                }
-            }
-        }
+        public virtual string Name => Prefix.Length == 0 ? LocalName : NameTable.Add(string.Concat(Prefix, ":", LocalName));


Suggested change

public virtual string Name => Prefix.Length == 0 ? LocalName : NameTable.Add(string.Concat(Prefix, ":", LocalName));

public virtual string Name => Prefix.Length == 0 ? LocalName : NameTable.Add($"{Prefix}:{LocalName}");

krwq · 2021-04-20T15:22:15Z


-        ReturnContent:
-            return (sb == null) ? value : sb.ToString();
+            ReturnContent:


I think previous indentation was correct (not 100% sure)

krwq · 2021-04-20T15:28:56Z

                        break;
                    case XmlNodeType.EntityReference:
-                        xtw.WriteEntityRef(this.Name);
+                        xtw.WriteEntityRef(Value);


this is not cleanup

krwq · 2021-04-20T15:29:53Z

-                {
-                    throw new XmlException(SR.Xml_ReadContentAsFormatException, returnType.ToString(), e, this as IXmlLineInfo);
-                }
+                return XmlUntypedConverter.Untyped.ChangeType(strContentValue, returnType, namespaceResolver ?? this as IXmlNamespaceResolver);


please move any implementation changes to a separate PR

krwq · 2021-04-20T15:30:10Z

            }

-            return (returnType == typeof(string)) ? string.Empty : XmlUntypedStringConverter.Instance.FromString(string.Empty, returnType, namespaceResolver);
+            return returnType == typeof(string) ? string.Empty :  XmlUntypedConverter.Untyped.ChangeType(string.Empty, returnType, namespaceResolver);


krwq · 2021-04-20T15:31:23Z

 // The .NET Foundation licenses this file to you under the MIT license.

-using System.Collections;
+using System.ComponentModel;


why is new namespace needed?

[System.ComponentModel.EditorBrowsable(System.ComponentModel.EditorBrowsableState.Never)] -> [EditorBrowsable(EditorBrowsableState.Never)]

krwq

Please move any implementation changes to a separate PR

jkotas · 2021-04-20T15:53:09Z


-        ReturnContent:
-            return (sb == null) ? value : sb.ToString();
+            ReturnContent:


This change goes against the coding style: https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/coding-style.md

When using labels (for goto), indent the label one less than the current indentation.

stephentoub

Style changes generally look fine. I agree with @krwq the functional changes need to be separated out.

danmoseley · 2021-04-21T00:21:29Z

That's a lot of semicolons, does that get us closer to enabling SA1106 globally?

stephentoub · 2021-04-21T00:59:08Z

That's a lot of semicolons, does that get us closer to enabling SA1106 globally?

Semicolons?

Regardless, we won't be enabling SA1106:
#50236 (comment)

danmoseley · 2021-04-21T03:36:43Z

Semicolons?

I was referring to the many empty statements that @kronic removed.

kronic · 2021-04-21T06:51:41Z

@krwq done

krwq · 2021-04-21T07:53:44Z

            }

-            return (returnType == typeof(string)) ? string.Empty : XmlUntypedStringConverter.Instance.FromString(string.Empty, returnType, namespaceResolver);
+            return returnType == typeof(string) ? string.Empty : XmlUntypedStringConverter.Instance.FromString(string.Empty, returnType, namespaceResolver!);


why is ! needed now?

Resharper gives warning Possible 'null' assignment to entity.
Next pr I plan to replace to XmlUntypedConverter.Untyped.ChangeType(string.Empty, returnType, namespaceResolver) as done in XmlRederAsync.ReadElementContentAsAsync

@krwq If necessary, can I remove it?

@kronic, I assume if it wasn't needed before it shouldn't be now, especially namespaceResolver is non-nullable in that context. For next PR, let's talk about it in next PR, this is likely ok but prefer to think about it separately (also easier to track down if we regress something)

krwq

LGTM minus ! comment

kronic · 2021-04-21T09:22:50Z

@krwq done

krwq · 2021-04-21T09:48:34Z

Thanks @kronic!

ghost · 2021-04-21T09:48:52Z

Hello @krwq!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

kronic · 2021-04-22T10:22:55Z

@krwq what are the problems of auto-merge

krwq · 2021-04-22T16:17:30Z

Seems CI leg is failing, it will not auto-merge until that pass. I'll see if the issue is related

EDIT: seems it's not. I'll restart failing legs, if this keeps on failing and it's known issue I'll merge manually

kronic added 4 commits March 25, 2021 16:45

remove repetitions ';'

6e698aa

Merge branch 'main' of github.com:kronic/runtime

c6fe0ee

Cleanup XmlReader

5a7cccc

Cleanup XmlReaderAsync

9e286a7

ghost added the area-System.Xml label Apr 20, 2021

krwq reviewed Apr 20, 2021

View reviewed changes

krwq requested changes Apr 20, 2021

View reviewed changes

jkotas reviewed Apr 20, 2021

View reviewed changes

stephentoub reviewed Apr 20, 2021

View reviewed changes

review feedback

aa2af30

krwq reviewed Apr 21, 2021

View reviewed changes

krwq approved these changes Apr 21, 2021

View reviewed changes

review feedback

bf8efb7

krwq approved these changes Apr 21, 2021

View reviewed changes

krwq added the auto-merge label Apr 21, 2021

ghost merged commit 1132baa into dotnet:main Apr 22, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

ghost locked as resolved and limited conversation to collaborators Jun 19, 2021

This pull request was closed.

	public virtual string Name => Prefix.Length == 0 ? LocalName : NameTable.Add(string.Concat(Prefix, ":", LocalName));
	public virtual string Name => Prefix.Length == 0 ? LocalName : NameTable.Add($"{Prefix}:{LocalName}");

Uh oh!

Conversation

kronic commented Apr 20, 2021

Uh oh!

ghost commented Apr 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krwq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Apr 21, 2021

Uh oh!

stephentoub commented Apr 21, 2021

Uh oh!

danmoseley commented Apr 21, 2021

Uh oh!

kronic commented Apr 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kronic Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kronic Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krwq left a comment

Choose a reason for hiding this comment

Uh oh!

kronic commented Apr 21, 2021

Uh oh!

krwq commented Apr 21, 2021

Uh oh!

ghost commented Apr 21, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

kronic commented Apr 22, 2021

Uh oh!

krwq commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kronic Apr 21, 2021 •

edited

Loading

kronic Apr 21, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

krwq commented Apr 22, 2021 •

edited

Loading