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

fix: address incorrect unit long names #11

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

stiefeljackal
Copy link

@stiefeljackal stiefeljackal commented Aug 28, 2024

The main purpose of this PR is to address the incorrect long name usage for single values. There are very small fixes present in the PR that occurred as a side effect of fixing the former. This small fixes will be listed in this PR.

With this PR, it will correctly display the singular form of the long name for a unit value (1 day, 1 gram, 1 meter, 1 yard, 1 thou, etc.).

Additionally, the follow small fixes are also present:

  • Replaced 'persec' with 'parsec' as the correct spelling.
  • Fixed mass unit "Ton" issues.
    • Fixed imperial ton and metric ton name mixup.
    • Used Tonne property for metric ton and marked Ton property as deprecated.
    • Added the US imperial ton (also known as short ton).

I also added unit tests to verify validity of the changes.

Fixes #10
Fixes #13
Fixes #14
Resolves #15

@ProbablePrime
Copy link
Member

This is a lot of work and looks Great.

I need some more time on my schedule to read it thoroughly, but wanted to leave thanks here to start! Thank you!

@stiefeljackal
Copy link
Author

stiefeljackal commented Aug 29, 2024

The packages folder was not properly cleaned out when I was verifying the build with dotnet the last time.

The csproj file was updated further to work with dotnet. dotnet was ran multiple times to verify, so the build job should not fail this time.

@stiefeljackal
Copy link
Author

I went back into the PR to review the algorithm for generating unit names and improved it since I did not like my original implementation. Setting the default short, long singular, and long plural names also happens when the array for long names and short names is set instead of relying on the GetUnitNames method call.

Overall, I think this implementation for it is better. The time it took for the unit tests to complete is still the same as before.

@stiefeljackal
Copy link
Author

I have retroactively created issues that addressed the small fixes mentioned in my first post as that should be documented in Issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants