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

Use International Mile for UnitLength operations #3033

Closed
wants to merge 3 commits into from

Conversation

nVitius
Copy link

@nVitius nVitius commented Aug 10, 2021

I made this PR based on some conversation in this SO post. This patch changes the Coefficient for the mile to 1609.344, to be in-line with the official definition of the "international mile".

From the NIST's Conversion Factor's for General Use handbook:
image

The existing Coefficient leads to an incorrect result. This can be seen by trying to convert 1 mile to feet (1 mile should be 5280 feet):
1 * 1609.34 / 0.3048 = 5279.9868766404199475065616797900262467191601049868766404199475065...
Using the standard 1609.344 instead gives 5280.

The Coefficients for inches, feet, and yards are all correct.

@spevans
Copy link
Collaborator

spevans commented Aug 11, 2021

@swift-ci test

@nVitius
Copy link
Author

nVitius commented Aug 11, 2021

@spevans @amomchilov
I believe I fixed the failing test cases.

I'm not 100% sure about the TestFoundation.TestMeasurement-testLoadedValuesMatch test.

It uses the .archive fixtures. As far as I can tell, those were created with NSKeyedArchiver. I'm not sure how to replicate that, but I think it's just a binary plist. I converted to XML and back to binary after updating the incorrect property (the coefficient for mile).

Will you please re-run the tests?

@spevans
Copy link
Collaborator

spevans commented Aug 12, 2021

@swift-ci test

@nVitius
Copy link
Author

nVitius commented Aug 17, 2021

Looks like the changes allowed the tests to pass.

@amomchilov @spevans
Not sure what the protocol is for merging PRs. Just pinging you two to make sure you've seen this. Let me know if you need anything else from me. Thanks!

@millenomi
Copy link
Contributor

@nVitius does your patch correspond to Darwin behavior? We try to keep things working 1:1 bug-for-bug where possible.

@nVitius
Copy link
Author

nVitius commented Aug 21, 2021

@millenomi Looking through the docs, it would seem like Darwin also has this issue:
https://developer.apple.com/documentation/foundation/unitlength?changes=latest_minor
image

I sent a report via FeedbackAssistant for now; I'm not sure where the best place to report this would be.

@amomchilov
Copy link

We try to keep things working 1:1 bug-for-bug where possible.

Oh jeeze. That's unfortunate, but understandable.

I've had better success piping my bug reports to /dev/null than sending them into feedback assistant 😅

@millenomi
Copy link
Contributor

sighs

Can you post your FB number so I can pass it on internally?

@amomchilov
Copy link

Pinging @nVitius, he's the one who submitted a bug report.

@nVitius
Copy link
Author

nVitius commented Aug 21, 2021

The FB number is: FB9542852

@millenomi
Copy link
Contributor

Thank you. I'm going to close this for now, but it was important and I'll make sure the issue is seen so we can ultimately fix it here as well.

@millenomi millenomi closed this Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants