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 Angstroms Length-Converter option #2229

Merged
merged 6 commits into from
Sep 24, 2024
Merged

Conversation

DevBoiAgru
Copy link
Contributor

@DevBoiAgru DevBoiAgru commented Sep 9, 2024

Fixes #2175

Description of the changes:

  • Adds Angstrom as a unit of length
  • Angstrom can be converted to other units and vice versa

How changes were validated:

  • Added unit testing

Adds Angstrom as a length unit to the list of unit constants in UnitConverterDataConstants.h file.
Adds the Angstrom unit of length to the list of length units in UnitConverterDataLoader.cpp and its conversion data is added to the GetConversionData() function.

It is assigned the value of 15.
Adds new data entries to Resources.resw for unit abbreviation of Angstrom, and the unit name for Angstrom.
Adds new data entry for "Meters-Angstroms" to Test.resw file
This commit changes the order of the units of length so that Angstrom is before Nanometers.
Increments orders of all other units by 1 and sets order of Angstroms to 1.
@DevBoiAgru
Copy link
Contributor Author

@microsoft-github-policy-service agree

@DevBoiAgru
Copy link
Contributor Author

Hi @tian-lt I hope you're doing well, just wanted to ask if this small MR could be merged? It's a small and straightforward update.

Thanks for your time! 😊

@tian-lt
Copy link
Contributor

tian-lt commented Sep 23, 2024

Hi @DevBoiAgru, thank you for proposing this PR. Most of the changes look good to me. However, there's a test case failure found by the pipeline.
It seems the MultipleConverterModeCalculationTest case may need some adjusts for the new unit.
Could you take a look? Let me know when you fixed it, and then I'll merge the PR if everything goes well.

@DevBoiAgru
Copy link
Contributor Author

DevBoiAgru commented Sep 23, 2024

@tian-lt Thanks for the reply. How could I run the tests locally on my machine so that I can mention you when it's done? I think the test case should be an easy fix with the order, so I dont want to push small changes again and again until it works

Update: Nevermind i think i figured it out, i'll keep you updated

@tian-lt
Copy link
Contributor

tian-lt commented Sep 23, 2024

Sure. You can run those unit tests by building the unit test project locally, which is located here: https://github.com/microsoft/calculator/tree/main/src/CalculatorUnitTests
And, you may manually add a self-signed certificate to pack the unit test project to install it on your machine and then run for testing. The way to do that is exactly the same as what packing a side-load UWP app needs.
Feel free to let me know if you need helps. @DevBoiAgru

This commit fixes the "TestStandardUnitConverterAndDateViewModels" failing by introducing a new resource entry to the Test.resw file.

These changes needed to be made because adding the new unit changed the order of units, hence the test case expected a different value, since it was converting other units.
@DevBoiAgru
Copy link
Contributor Author

DevBoiAgru commented Sep 23, 2024

@tian-lt Apologies for another mention but the most recent commit should fix the test case not passing. I have modified a test case so that it checks for the correct units. Thank you.

@tian-lt
Copy link
Contributor

tian-lt commented Sep 23, 2024

LGTM.
@hanzhang54 do you have any comments on this?

@tian-lt tian-lt merged commit ff16a72 into microsoft:main Sep 24, 2024
5 checks passed
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.

Add angstroms as convertible length unit
2 participants