Skip to content

Fix order of line number/position arguments to XsltException ctor#63344

Merged
krwq merged 1 commit intodotnet:mainfrom
stephentoub:xsltexceptionorder
Jan 4, 2022
Merged

Fix order of line number/position arguments to XsltException ctor#63344
krwq merged 1 commit intodotnet:mainfrom
stephentoub:xsltexceptionorder

Conversation

@stephentoub
Copy link
Member

No description provided.

@ghost ghost added the area-System.Xml label Jan 4, 2022
@ghost ghost assigned stephentoub Jan 4, 2022
@ghost
Copy link

ghost commented Jan 4, 2022

Tagging subscribers to this area: @dotnet/area-system-xml
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: stephentoub
Assignees: -
Labels:

area-System.Xml

Milestone: -

public override void CheckErrors()
{
throw new XsltException(SR.Xslt_InvalidXPath, new string[] { Expression }, _baseUri, _linePosition, _lineNumber, null);
throw new XsltException(SR.Xslt_InvalidXPath, new string[] { Expression }, _baseUri, _lineNumber, _linePosition, null);
Copy link
Member

@krwq krwq Jan 4, 2022

Choose a reason for hiding this comment

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

Considering this only affects message suffix (An error occurred at {0}, ({1}, {2}).) it doesn't make much difference and hopefully people don't rely on that message and use properties instead. I'm wondering if for consistency with previous versions we should consider also swapping order when creating message: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.Xml/src/System/Xml/Xslt/XsltException.cs#L126

I don't feel strongly either way

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm wondering if for consistency with previous versions we should consider also swapping order when creating message

I'm not following. Can you elaborate?

Copy link
Member

Choose a reason for hiding this comment

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

with this change I assume we will swap the (col, row) in the error message with (row, col) for XSLT, it will be more consistent with rest of the XML but it will produce different message than on older versions.

Copy link
Member

Choose a reason for hiding this comment

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

In all error/warning messages I am familiar with across various platforms, (row, col) seems to be the standard. It's also assumed by various tools such as VS. So it's the right fix IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

with this change I assume we will swap the (col, row) in the error message with (row, col) for XSLT, it will be more consistent with rest of the XML but it will produce different message than on older versions.

no, pr is simply fixing 4th and 5th arguments which were wrong before:

internal XsltException(string res, string?[] args, string? sourceUri, int lineNumber, int linePosition, Exception? inner)

Copy link
Member

@krwq krwq left a comment

Choose a reason for hiding this comment

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

I'm ok with this change as it makes error message more consistent across XML

@krwq krwq merged commit 932c822 into dotnet:main Jan 4, 2022
@stephentoub stephentoub deleted the xsltexceptionorder branch January 4, 2022 17:56
@ghost ghost locked as resolved and limited conversation to collaborators Feb 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants