Skip to content

Conversation

@ChrisMaddock
Copy link
Contributor

Properties in the remarks section of this API page were inconsistently fenced/xref'd. I assume there's no problem xref'ing everything!

Copy link

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

Thanks for replacing fenced text with links, @ChrisMaddock. One of the links is broken, though -- ParamName is a property of ArgumentException, not Exception.

|`Message`|The error message string.|
|`ParamName`|The parameter name string.|
|<xref:System.Exception.Message%2A>|The localized error message string.|
|<xref:System.Exception.ParamName%2A>|The parameter name string.|

Choose a reason for hiding this comment

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

This should be <xref:System.ArgumentException.ParamName%2A>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @rpetrusha - good spot!

Now you've pointed it out - I've noticed the other xref's are inconsistent in the file. Some reference <xref:System.ArgumentException.Message%2A>, others <xref:System.Exception.Message%2A>. Should they all be referencing ArgumentException, or does it not matter?

Choose a reason for hiding this comment

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

That's a good question, @ChrisMaddock . They actually all should reference ArgumentException, since it overrides the base class Exception.Message.

@mairaw
Copy link
Contributor

mairaw commented May 15, 2018

CLA seems stuck.

@mairaw mairaw closed this May 15, 2018
@mairaw mairaw reopened this May 15, 2018
@ChrisMaddock
Copy link
Contributor Author

@rpetrusha Additional changes made - would you be able to take another look?

Thanks for the clarification here. 😄

Copy link

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

Thanks for making all of the additional changes, @ChrisMaddock. This looks really good. We'll merge once the build completes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants