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

Converters between meters and feet (US Survey and International) added #935

Merged
merged 5 commits into from Oct 29, 2019

Conversation

qmark
Copy link
Contributor

@qmark qmark commented Oct 15, 2019

Description of Change

Converters between meters and feet (US Survey and International) added

API Changes

    public static double MetersToUSSurveyFeet(double meters);
    public static double USSurveyFeetToMeters(double usfeet);

    public static double MetersToInternationalFeet(double meters);
    public static double InternationalFeetToMeters(double intfeet);

@VSC-Service-Account
Copy link

Docs Build status updates of commit bf4e1ac:

⚠️ Validation status: warnings

File Status Preview URL Details
⚠️Warning Details
Xamarin.Essentials/Types/UnitConverters.shared.cs ✅Succeeded
docs/en/Xamarin.Essentials/SizeExtensions.xml ✅Succeeded
docs/en/Xamarin.Essentials/RectangleExtensions.xml ✅Succeeded
docs/en/Xamarin.Essentials/PointExtensions.xml ✅Succeeded
docs/en/Xamarin.Essentials/ColorExtensions.xml ✅Succeeded

  • [Warning] Uid(Xamarin.Essentials.ColorExtensions.ToPlatformColor(System.Drawing.Color)_m) has already been defined in api/Xamarin.Essentials.ColorExtensions.ToPlatformColor.yml#Xamarin_Essentials_ColorExtensions_ToPlatformColor_System_Drawing_Color__m.
  • [Warning] Uid(Xamarin.Essentials.PointExtensions.ToPlatformPoint(System.Drawing.Point)_m) has already been defined in api/Xamarin.Essentials.PointExtensions.ToPlatformPoint.yml#Xamarin_Essentials_PointExtensions_ToPlatformPoint_System_Drawing_Point__m.
  • [Warning] Uid(Xamarin.Essentials.SizeExtensions.ToPlatformSize(System.Drawing.Size)_m) has already been defined in api/Xamarin.Essentials.SizeExtensions.ToPlatformSize.yml#Xamarin_Essentials_SizeExtensions_ToPlatformSize_System_Drawing_Size__m.
  • [Warning] Uid(Xamarin.Essentials.RectangleExtensions.ToPlatformRectangle(System.Drawing.Rectangle)_m) has already been defined in api/Xamarin.Essentials.RectangleExtensions.ToPlatformRectangle.yml#Xamarin_Essentials_RectangleExtensions_ToPlatformRectangle_System_Drawing_Rectangle__m.

For more details, please refer to the build report.

Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report.

mattleibow
mattleibow previously approved these changes Oct 16, 2019
Copy link
Contributor

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

LGTM

@jamesmontemagno
Copy link
Collaborator

Is naming alright @Redth thoughts @mattleibow does everything align?

Looks like there is some spacing issues:

Xamarin.Essentials\Types\UnitConverters.shared.cs(105,1): Error SA1516: Elements should be separated by blank line

@jamesmontemagno
Copy link
Collaborator

Also needs documentation:

1 Foot:
International survey foot defined as exactly 0.3048 meters by convention in 1959. This is the most common modern foot measure. US Survey may also be used but are nearly identical for short distances. 1 ft = 0.3048 m.

1 Foot (U.S. Survey):
Exactly 1200/3937 meters by definition. In decimal terms approximately 0.304 800 609 601 219 meters. Variation from the common international foot of exactly 0.3048 meters may only be considerable over large survey distances. 1 ft (US survey) = 1200/3937 m

Should there also be.... USSurveyFeetToInternationalFeet and InternationalFeetToUSSurveyFeet ?

@jamesmontemagno
Copy link
Collaborator

Also needs tests as to make sure they are being converted correctly

@VSC-Service-Account
Copy link

Docs Build status updates of commit b1e5cf5:

🕙 Pending: waiting for processors (6 builds ahead of you)

⚠️ Docs Build is busy, currently there are 6 builds ahead of this one, for more information you can view the Build queue graph on the Docs Portal.

@msftclas
Copy link

msftclas commented Oct 16, 2019

CLA assistant check
All CLA requirements met.

@VSC-Service-Account
Copy link

Docs Build status updates of commit b1e5cf5:

⚠️ Validation status: warnings

File Status Preview URL Details
⚠️Warning Details
Xamarin.Essentials/Types/UnitConverters.shared.cs ✅Succeeded
docs/en/Xamarin.Essentials/SizeExtensions.xml ✅Succeeded
docs/en/Xamarin.Essentials/RectangleExtensions.xml ✅Succeeded
docs/en/Xamarin.Essentials/PointExtensions.xml ✅Succeeded
docs/en/Xamarin.Essentials/ColorExtensions.xml ✅Succeeded

  • [Warning] Uid(Xamarin.Essentials.SizeExtensions.ToPlatformSize(System.Drawing.Size)_m) has already been defined in api/Xamarin.Essentials.SizeExtensions.ToPlatformSize.yml#Xamarin_Essentials_SizeExtensions_ToPlatformSize_System_Drawing_Size__m.
  • [Warning] Uid(Xamarin.Essentials.ColorExtensions.ToPlatformColor(System.Drawing.Color)_m) has already been defined in api/Xamarin.Essentials.ColorExtensions.ToPlatformColor.yml#Xamarin_Essentials_ColorExtensions_ToPlatformColor_System_Drawing_Color__m.
  • [Warning] Uid(Xamarin.Essentials.PointExtensions.ToPlatformPoint(System.Drawing.Point)_m) has already been defined in api/Xamarin.Essentials.PointExtensions.ToPlatformPoint.yml#Xamarin_Essentials_PointExtensions_ToPlatformPoint_System_Drawing_Point__m.
  • [Warning] Uid(Xamarin.Essentials.RectangleExtensions.ToPlatformRectangle(System.Drawing.Rectangle)_m) has already been defined in api/Xamarin.Essentials.RectangleExtensions.ToPlatformRectangle.yml#Xamarin_Essentials_RectangleExtensions_ToPlatformRectangle_System_Drawing_Rectangle__m.

For more details, please refer to the build report.

Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report.

@qmark
Copy link
Contributor Author

qmark commented Oct 16, 2019

@jamesmontemagno have added documentation and refactored for a nicer look (with constants and indents). Unfortunately I cannot launch Tests of Essentials projects, but I can provide NUnit tests I use for the code in my project (and they run successively)

    [Test]
    public void MetersToInternationalFeet()
    {
        Assert.AreEqual(10000, DistanceUtility.MetersToInternationalFeet(3048));
    }

    [Test]
    public void InternationalFeetToMeters()
    {
        Assert.AreEqual(6096, DistanceUtility.InternationalFeetToMeters(20000));
    }

    [Test]
    public void MetersToUSSurveyFeet()
    {
        Assert.AreEqual(3937, DistanceUtility.MetersToUSSurveyFeet(1200));
    }

    [Test]
    public void USSurveyFeetToMeters()
    {
        Assert.AreEqual(2400, DistanceUtility.USSurveyFeetToMeters(7874));
    }

@Redth
Copy link
Member

Redth commented Oct 16, 2019

@qmark can you please change these to use xUnit ?

[Fact]
public void USSurveyFeetToMeters()
{
    Assert.AreEqual(2400, DistanceUtility.USSurveyFeetToMeters(7874));
}

Thanks

@qmark
Copy link
Contributor Author

qmark commented Oct 16, 2019

@Redth I will, however I cannot launch them in the project, so not sure if they will run successfully. Does autocheck run unit tests?

@VSC-Service-Account
Copy link

Docs Build status updates of commit 258b7f6:

⚠️ Validation status: warnings

File Status Preview URL Details
⚠️Warning Details
Tests/UnitConverters_Tests.cs ✅Succeeded
Xamarin.Essentials/Types/UnitConverters.shared.cs ✅Succeeded
docs/en/Xamarin.Essentials/SizeExtensions.xml ✅Succeeded
docs/en/Xamarin.Essentials/RectangleExtensions.xml ✅Succeeded
docs/en/Xamarin.Essentials/PointExtensions.xml ✅Succeeded
docs/en/Xamarin.Essentials/ColorExtensions.xml ✅Succeeded

  • [Warning] Uid(Xamarin.Essentials.PointExtensions.ToPlatformPoint(System.Drawing.Point)_m) has already been defined in api/Xamarin.Essentials.PointExtensions.ToPlatformPoint.yml#Xamarin_Essentials_PointExtensions_ToPlatformPoint_System_Drawing_Point__m.
  • [Warning] Uid(Xamarin.Essentials.ColorExtensions.ToPlatformColor(System.Drawing.Color)_m) has already been defined in api/Xamarin.Essentials.ColorExtensions.ToPlatformColor.yml#Xamarin_Essentials_ColorExtensions_ToPlatformColor_System_Drawing_Color__m.
  • [Warning] Uid(Xamarin.Essentials.SizeExtensions.ToPlatformSize(System.Drawing.Size)_m) has already been defined in api/Xamarin.Essentials.SizeExtensions.ToPlatformSize.yml#Xamarin_Essentials_SizeExtensions_ToPlatformSize_System_Drawing_Size__m.
  • [Warning] Uid(Xamarin.Essentials.RectangleExtensions.ToPlatformRectangle(System.Drawing.Rectangle)_m) has already been defined in api/Xamarin.Essentials.RectangleExtensions.ToPlatformRectangle.yml#Xamarin_Essentials_RectangleExtensions_ToPlatformRectangle_System_Drawing_Rectangle__m.

For more details, please refer to the build report.

Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report.

@qmark
Copy link
Contributor Author

qmark commented Oct 16, 2019

Managed to run the tests, they all pass

@qmark qmark requested a review from mattleibow October 18, 2019 11:49
@Redth
Copy link
Member

Redth commented Oct 28, 2019

@qmark the only other question I'd say is, do we really need to call them International feet? Is this not the commonly understood implementation of feet already? I may be ignorant here but to me it seems more confusion to call them International Feet. I totally get calling the US Survey feet specifically out though.

Thoughts?

@Redth Redth changed the base branch from master to dev/1.4.0 October 28, 2019 19:10
@Redth Redth added this to the 1.4.0 milestone Oct 28, 2019
@VSC-Service-Account
Copy link

Docs Build status updates of commit a27f831:

✅ Validation status: passed

File Status Preview URL Details
Tests/UnitConverters_Tests.cs ✅Succeeded
Xamarin.Essentials/Types/UnitConverters.shared.cs ✅Succeeded

For more details, please refer to the build report.

Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report.

@qmark
Copy link
Contributor Author

qmark commented Oct 29, 2019

@Redth Yes, it is the commonly understood implementation of feet, and I also added summary with "This is the most common modern foot measure" for International foot. For me it's hard to say if it is confusing, as I have some knowledge in the field, and say there is also Indian survey foot, which may be just "Foot" for someone in India, or metric foot, which is exactly 30 cm and may be "Foot" for someone in Europe.

I'd say it's better for someone in the U.S. to have it as just "Foot", but not sure about other people. For instance, English wiki page https://en.wikipedia.org/wiki/Foot_(unit) says "one foot is defined as 0.3048 meter exactly" in the second sentence, but Russian wiki https://ru.wikipedia.org/wiki/Фут says "The exact linear value varies from country to country." in the second sentence, and "“International” foot began to equal exactly 0.3048 m" later on.

@Redth Redth merged commit 4b12a72 into xamarin:dev/1.4.0 Oct 29, 2019
@Redth
Copy link
Member

Redth commented Oct 29, 2019

Thanks!

@VSC-Service-Account
Copy link

Docs Build status updates of commit e19ed72:

✅ Validation status: passed

File Status Preview URL Details
Tests/UnitConverters_Tests.cs ✅Succeeded
Xamarin.Essentials/Types/UnitConverters.shared.cs ✅Succeeded

For more details, please refer to the build report.

Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report.

@qmark qmark deleted the add-feet-convertors branch October 29, 2019 16:23
nickrandolph pushed a commit to builttoroam/Essentials that referenced this pull request Dec 4, 2019
xamarin#935)

* Converters between meters and feet (US Survey and International) added

* Refactoring and documentation

* xUnit tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants