-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add span-based methods to Decimal #32155
Conversation
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.
Some comments on tests primarily, otherwise LGTM.
|
fyi @carlossanlop, @maryamariyan - label for "new api needs doc" missing. |
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.
Otherwise, looks good.
...ries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/DecimalUtilities.cs
Show resolved
Hide resolved
There are a few other places where we could call the new API. Did you intentionally exclude them: For example: runtime/src/libraries/System.Data.Odbc/src/System/Data/Odbc/OdbcParameterHelper.cs Line 232 in 4f9ae42
runtime/src/libraries/System.Private.Xml/src/System/Xml/Xsl/IlGen/GenerateHelper.cs Line 943 in 4f9ae42
|
Yes, two of those target netstandard2.0. The other one is a code gen tool for which we have no tests and in which such a change won't be at all measurable. |
Add missing tests
b84c3ab
to
4c7d3d0
Compare
4c7d3d0
to
664a9f9
Compare
@ahsonkhan I haven't enabled it on the bot yet. (happens once every two days instead) |
@ahsonkhan I just enabled auto-labeling PRs with "new-api-needs-documentation" label. Should show up on future PRs. |
Fixes #28904
cc: @tannergooding, @GrabYourPitchforks