Repository navigation
XmlReader.IsStartElement namespace argument should allow null #41461
Description
Background and Motivation
XmlReader has 3 overloads of IsStartElement:
runtime/src/libraries/System.Xml.ReaderWriter/ref/System.Xml.ReaderWriter.cs
Lines 805 to 807 in 46dd8ed
Calling the last overload with ns == null doesn't act like calling the middle overload. In other places in XmlReader, the "namespace" parameter is optional and marked with string?, but this one is being annotated as non-nullable because if you passed in null, it would always return false.
In the .NET Design Guidelines we have:
✔️ DO allow null to be passed for optional arguments.
We should consider changing this overload to be string? ns, and making this API act accordingly.
Proposed API
namespace System.Xml
{
public abstract class XmlReader
{
public virtual bool IsStartElement() { }
public virtual bool IsStartElement(string name) { }
- public virtual bool IsStartElement(string localname, string ns) { }
+ public virtual bool IsStartElement(string localname, string? ns) { } Usage Examples
XmlReader r = ...;
string? ns = GetOptionalNamespace();
r.IsStartElement("Customer", ns);As the API exists today, callers will instead be forced to check for null and call the other overload themselves:
XmlReader r = ...;
string? ns = GetOptionalNamespace();
if (ns == null)
{
r.IsStartElement("Customer");
}
else
{
r.IsStartElement("Customer", ns);
}
Activity
krwq commentedon Aug 28, 2020
@eerhardt one thing to note is that in case of overload without namespace the namespace is ignored. In case of this overload null may mean:
IsStartElement(string)would dowe should figure out how to interpret that and do some research on what customers would expect
KalleOlaviNiemitalo commentedon Aug 29, 2020
I don't think it should treat
string localname(local name) asstring name(qualified name) just becausestring nsisnull.If I'm writing an XML deserializer that should compare expanded names regardless of which namespace prefix is used, then I should avoid
IsStartElement(string name)and instead useIsStartElement(string localname, string ns). But ifns == nullcauses the latter to behave like the former, then it will be harder for me to be sure I did it right. I don't think there is a scenario in which the caller ofIsStartElementwould want a runtime decision on whether to use Namespaces in XML semantics or not.So, for
IsStartElement(localname, ns: null), my order of preference would be:falseregardless oflocalName-- current implementation, no compatibility risk, easy to documentArgumentNullException-- consistent withXNamespace.GetName, but would be a breaking changeIsStartElement(localName, ns: string.Empty)-- seems inconsistent with existing APIsIsStartElement(name: localName)-- does not seem useful; consistent with undocumented behavior ofXmlWriter.WriteStartElement(string localName, string ns)If the treatment of
string nsinXmlReader.IsStartElementis changed, thenXmlReader.ReadStartElementshould be changed similarly.krwq commentedon Nov 19, 2020
@eerhardt I generally agree with @KalleOlaviNiemitalo + what I wrote above, I'm leaning toward closing this as "won't fix" given the confusion this would cause. Please re-open if you feel strongly about fixing this.