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.

Sign up for GitHub

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

Jump to bottom

Use Math.CopySign in CalendricalCalculationsHelper #60776

Merged
merged 1 commit into from Oct 22, 2021
Merged

Use Math.CopySign in CalendricalCalculationsHelper #60776

merged 1 commit into from Oct 22, 2021

Conversation

ebraminio
Copy link
Contributor

This removes locally implemented CopySign in CalendricalCalculationsHelper
in favor of Math.CopySign.

The functionality of this class is solely in use in PersianCalendar class of
the same package and is fully tested in PersianCalendarTests so changing it
shouldn't impose any risk.

@ebraminio
Use Math.CopySign in CalendricalCalculationsHelper
1054f70 
This removes locally implemented CopySign in CalendricalCalculationsHelper
in favor of Math.CopySign.

The functionality of this class is solely in use in PersianCalendar class of
the same package and is fully tested in PersianCalendarTests so changing it
shouldn't impose any risk.
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 22, 2021
@ghost
Copy link

ghost commented Oct 22, 2021

Tagging subscribers to this area: @tarekgh, @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

This removes locally implemented CopySign in CalendricalCalculationsHelper
in favor of Math.CopySign.

The functionality of this class is solely in use in PersianCalendar class of
the same package and is fully tested in PersianCalendarTests so changing it
shouldn't impose any risk.

Author: ebraminio
Assignees: -
Labels:

area-System.Globalization, community-contribution
Milestone: -

@ghost ghost added this to Active PRs in ML, Extensions, Globalization, etc, POD. Oct 22, 2021
safern
safern approved these changes Oct 22, 2021
View reviewed changes
Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

LGTM

tarekgh
tarekgh approved these changes Oct 22, 2021
View reviewed changes
Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

@tannergooding the change looks good to me. Could you have a quick look just in case see any difference?

@tarekgh tarekgh added this to the 7.0.0 milestone Oct 22, 2021
@runfoapp runfoapp bot mentioned this pull request Oct 22, 2021
Closed
@tarekgh
Copy link
Member

tarekgh commented Oct 22, 2021

I logged the issue #60790 to track the unrelated failure in the CI leg runtime (CoreCLR Pri0 Runtime Tests Run Linux x64 checked)

@tarekgh tarekgh merged commit 18f3f56 into dotnet:main Oct 22, 2021
ML, Extensions, Globalization, etc, POD. automation moved this from Active PRs to Done Oct 22, 2021
@tarekgh
Copy link
Member

tarekgh commented Oct 22, 2021

Thanks @ebraminio for submitting this change.

@ebraminio ebraminio deleted the copysign branch October 22, 2021 23:18
@dotnet dotnet locked as resolved and limited conversation to collaborators Nov 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tannergooding tannergooding tannergooding approved these changes

@tarekgh tarekgh tarekgh approved these changes

@safern safern safern approved these changes

Assignees
No one assigned
Labels
area-System.Globalization community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ebraminio @tarekgh @tannergooding @safern