Skip to content

Add compressibility quantity #1037

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

Merged
merged 7 commits into from
May 4, 2022

Conversation

tmilnthorp
Copy link
Collaborator

Closes #1032

@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

Merging #1037 (1fb9f26) into master (53c9784) will decrease coverage by 0.1%.
The diff coverage is 80.8%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1037     +/-   ##
========================================
- Coverage    82.7%   82.6%   -0.0%     
========================================
  Files         308     310      +2     
  Lines       47569   47922    +353     
========================================
+ Hits        39293   39578    +285     
- Misses       8276    8344     +68     
Impacted Files Coverage Δ
UnitsNet/GeneratedCode/Quantity.g.cs 66.9% <66.7%> (-<0.1%) ⬇️
...sNet/GeneratedCode/Quantities/Compressibility.g.cs 80.6% <80.6%> (ø)
...neratedCode/NumberToCompressibilityExtensions.g.cs 100.0% <100.0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53c9784...1fb9f26. Read the comment docs.

Copy link
Owner

@angularsen angularsen left a comment

Choose a reason for hiding this comment

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

Found a minor typo, other than that you can go ahead and merge this

@angularsen
Copy link
Owner

Are you sure it's plural pounds in InversePoundsForceInches?
In Torque, we used PoundForceInches.
https://github.com/angularsen/UnitsNet/blob/master/Common/UnitDefinitions/Torque.json#L67-L79

I see both forms used, but I seem to find more singular form than plural form.
https://en.wikipedia.org/wiki/Pound_(force)

@tmilnthorp
Copy link
Collaborator Author

Are you sure it's plural pounds in InversePoundsForceInches? In Torque, we used PoundForceInches. https://github.com/angularsen/UnitsNet/blob/master/Common/UnitDefinitions/Torque.json#L67-L79

I see both forms used, but I seem to find more singular form than plural form. https://en.wikipedia.org/wiki/Pound_(force)

This is more associated with Pressure than Torque. That said, I'd love for @sveipa to clarify

@stale
Copy link

stale bot commented Apr 18, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Apr 18, 2022
@sveipa
Copy link

sveipa commented Apr 19, 2022

Are you sure it's plural pounds in InversePoundsForceInches? In Torque, we used PoundForceInches. https://github.com/angularsen/UnitsNet/blob/master/Common/UnitDefinitions/Torque.json#L67-L79
I see both forms used, but I seem to find more singular form than plural form. https://en.wikipedia.org/wiki/Pound_(force)

This is more associated with Pressure than Torque. That said, I'd love for @sveipa to clarify

I've run the singular vs plural case internally here without being able to conclude what's the right naming convention unfortunately. Sorry for not being able to help on this matter.

@stale stale bot removed the wontfix label Apr 19, 2022
@angularsen
Copy link
Owner

If there is no clear cut convention, I vote we use singular form InversePoundForceInches since it is consistent with PoundForceInches. I don't have strong feelings either way, but I prefer consistency.

@sveipa
Copy link

sveipa commented Apr 29, 2022

Can this PR be merged please? :-)

@angularsen
Copy link
Owner

I checked the code base and we have 40/60 mix of PoundForce vs PoundsForce, so I guess there is no consistency to begin with. Keeping it PoundsForce then.

@angularsen angularsen merged commit 1e1f21b into angularsen:master May 4, 2022
@angularsen
Copy link
Owner

Sorry for the slow turnaround @sveipa @tmilnthorp , nuget is on the way out.

Release UnitsNet/4.129.0 · angularsen/UnitsNet

@sveipa
Copy link

sveipa commented May 6, 2022

Great, thanks @angularsen :-)

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.

3 participants