-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Conversation
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.
@microsoft-github-policy-service agree |
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! 😊 |
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. |
@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 |
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 |
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.
@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. |
LGTM. |
Fixes #2175
Description of the changes:
How changes were validated: