Skip to content
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 Zero and IsZero properties to Quaternion. #57354

Closed
wants to merge 5 commits into from

Conversation

Takym
Copy link
Contributor

@Takym Takym commented Aug 13, 2021

This pull request resolves #57253.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 13, 2021
@ghost
Copy link

ghost commented Aug 13, 2021

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

Issue Details

This pull request resolves #57253.

Author: Takym
Assignees: -
Labels:

area-System.Numerics

Milestone: -

@GrabYourPitchforks
Copy link
Member

Hi @Takym! Thanks for the PR. Unfortunately the original issue is not yet marked api-approved, so we can't accept this PR at this time. See https://github.com/dotnet/runtime/blob/main/docs/project/api-review-process.md#pull-requests for more information on the API approval process and the timelines for when PRs are accepted.

@Takym
Copy link
Contributor Author

Takym commented Aug 13, 2021

Hello @GrabYourPitchforks! Thank you for replying. Do I need to re-create a PR when the original issue is marked as api-approved, or will you reopen this PR?

@GrabYourPitchforks
Copy link
Member

I think you have the power to reopen the PR yourself at the right time. If not, leave a comment here and somebody should be able to reopen it on your behalf. The system won't auto-lock the issue for quite some time.

@Takym
Copy link
Contributor Author

Takym commented Aug 15, 2021

I clearly understood. Thanks.

@Takym
Copy link
Contributor Author

Takym commented Aug 17, 2021

Would you please reopen this PR because the issue was marked as api-approved?

@GrabYourPitchforks
Copy link
Member

GrabYourPitchforks commented Aug 17, 2021

@Takym Reopening. Only the Zero property was approved in API review; there didn't seem to be much love for an IsZero property.

Don't forget to update the ref assemblies and provide unit tests for the new API while you're at it. :)

Edit: Looks like you already beat me to the unit tests!

@Takym
Copy link
Contributor Author

Takym commented Aug 17, 2021

I added property definitions to the reference assembly in 7a54468.

Also, I have created a "no IsZero" version below:
Takym/runtime@Quaternion...Quaternion2

@stephentoub
Copy link
Member

This can be closed now that #58406 has been opened, yes?

@Takym
Copy link
Contributor Author

Takym commented Sep 1, 2021

@stephentoub Yes.

@Takym Takym closed this Sep 1, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Oct 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Numerics community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Quaternion.Zero is missing
3 participants