-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Specify XmlException LineNumber and LinePosition are zero-based #3636
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
Conversation
@carlossanlop I believe we are starting to count with 1. The only thing to mention is that 0 can be encountered when user chooses to not use line info (which is the default). Specific line of code which is mentioned above Easiest way to check: using System;
using System.Collections.Generic;
using System.Linq;
using System.Xml;
using System.Xml.Linq;
using System.IO;
using System.Text;
public class Program
{
public static void Main()
{
using (MemoryStream ms = new MemoryStream(Encoding.UTF8.GetBytes("<foo>\r\n<ba</foo>")))
{
XDocument xdoc = XDocument.Load(ms, LoadOptions.SetLineInfo);
}
}
} and observe exception:
|
@krwq thank you for the clarification. I'll adjust the descriptions. |
Here's an example that throws on line 1: using System.IO;
using System.Text;
using System.Xml.Linq;
namespace ConsoleApp
{
public class Program
{
public static void Main()
{
using (MemoryStream ms = new MemoryStream(Encoding.UTF8.GetBytes("<<foo>\r\nbar</foo>")))
{
XDocument xdoc = XDocument.Load(ms, LoadOptions.SetLineInfo);
}
}
}
} Exception message: |
@mairaw I'm experimenting with adding an example in the Remarks section (without the Remarks title). I recall a few examples where we've done this, and Ron approved a couple of them. What do you think? |
It should be fine. I left a couple of comments for you to consider and apply to the other API as well. |
Co-Authored-By: Maira Wenzel <mairaw@microsoft.com>
Here's the preview, @mairaw: https://review.docs.microsoft.com/en-us/dotnet/api/system.xml.xmlexception.linenumber?view=netcore-3.0&branch=pr-en-us-3636#System_Xml_XmlException_LineNumber What do you think? Looks good to merge? |
I'll make some suggestions. Since what you're showing is anti-pattern, we probably should have this in the remarks instead and just show the little snippet that throws the exception. Makes sense? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some suggestions for you to consider.
Co-Authored-By: Maira Wenzel <mairaw@microsoft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't think you need the remarks but I'll let you decide that. Left some comments in case you decide to keep them.
Co-Authored-By: Maira Wenzel <mairaw@microsoft.com>
Fixes: #851
Area owners: @buyaa-n and @krwq