Skip to content

Conversation

@pgovind
Copy link

@pgovind pgovind commented Sep 7, 2021

As part of running CA2252 locally to test the dotnet/runtime build, the CA2252 analyzer tagged these APIs as being implicit implementations that didn't have the [RequiresPreviewFeatures] attribute. This PR fixes that.

cc @jeffhandley

@ghost
Copy link

ghost commented Sep 7, 2021

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Sep 7, 2021

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@pgovind pgovind marked this pull request as draft September 7, 2021 23:05
@pgovind pgovind changed the title Add explicit APIs to Double/Single/Half/TimeOnly [DO NOT REVIEW YET] Add explicit APIs to Double/Single/Half/TimeOnly Sep 7, 2021
@pgovind pgovind changed the title [DO NOT REVIEW YET] Add explicit APIs to Double/Single/Half/TimeOnly Add explicit APIs to Double/Single/Half/TimeOnly Sep 7, 2021
@pgovind pgovind marked this pull request as ready for review September 7, 2021 23:17
=> Math.Truncate(x);

[RequiresPreviewFeatures]
static bool IFloatingPoint<double>.IsFinite(double d) => IsFinite(d);
Copy link
Author

Choose a reason for hiding this comment

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

I didn't add tests for the new APIs since I didn't find tests for the other generic math APIs in Double/Single/Half. I assumed we'll add them as part of adding other unit tests here.

@jeffhandley jeffhandley added this to the 6.0.0 milestone Sep 7, 2021
Copy link
Member

@jeffhandley jeffhandley 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 handling this, @pgovind. Once we get this merged into main, we'll want to backport this into release/6.0 and submit it for RC2 approval.

@ghost
Copy link

ghost commented Sep 7, 2021

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

Issue Details

As part of running CA2252 locally to test the dotnet/runtime build, the CA2252 analyzer tagged these APIs as being implicit implementations that didn't have the [RequiredPreviewFeatures] attribute. This PR fixes that.

cc @jeffhandley

Author: pgovind
Assignees: -
Labels:

area-System.Numerics, new-api-needs-documentation

Milestone: -

@jeffhandley
Copy link
Member

The failing check is from System.Net.Http.WinHttpHandler.Functional.Tests and it was an unrelated networking issue.

@jeffhandley

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@jeffhandley
Copy link
Member

@danmoseley I'm not sure how to resolve or work around the license/cla check failing to finish. Aside from that this is ready to merge. Do you know how to get past that?

@pgovind
Copy link
Author

pgovind commented Sep 8, 2021

@Anipik, @ViktorHofer, @safern : Any idea how to get past the license/cla check? I don't see a re-run option for that leg.

@danmoseley
Copy link
Member

@dotnet/dnceng any idea why license/cla got jammed? Its not this PR that did it.

@pgovind there is always the close/reopen option ..

@missymessa
Copy link
Member

@dotnet/dnceng any idea why license/cla got jammed? Its not this PR that did it.

@pgovind there is always the close/reopen option ..

This has happened occasionally (I found this issue a while ago cla-assistant/cla-assistant#528). Closing and reopening the PR is probably the best way to mitigate this issue.

@danmoseley
Copy link
Member

@missymessa thanks ... is it worth you opening a new issue there?

@danmoseley danmoseley closed this Sep 8, 2021
@danmoseley danmoseley reopened this Sep 8, 2021
@missymessa
Copy link
Member

It appears there is another open issue where lots of folks are reporting seeing the issue: cla-assistant/cla-assistant#520

@pgovind
Copy link
Author

pgovind commented Sep 8, 2021

@pgovind there is always the close/reopen option ..

Yea, I considered that, but decided that one of the infra folks using their special powers to merge the PR would be better :) I see you closed and re-opened the PR, so never mind!

@jeffhandley
Copy link
Member

The CI checks had previously finished, so I'm going to go ahead and merge.

@jeffhandley jeffhandley merged commit f7b441d into dotnet:main Sep 8, 2021
@danmoseley
Copy link
Member

one of the infra folks using their special powers to merge the PR would be better

I too have such special powers BTW. But for something like "licensing/CLA" it is probably good to make certain it runs.

@jeffhandley
Copy link
Member

/backport to release/6.0

@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2021

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1215421182

@safern
Copy link
Member

safern commented Sep 21, 2021

FYI: since opening and reopening triggers all other legs, the best way to trigger the CLA run when it doesn't run is using the following address: https://cla.dotnetfoundation.org/check/%org%/%repo%?pullRequest=%pr%

@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
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.

5 participants