Contribution Guideline #328

ferittuncer · 2017-11-24T15:19:27Z

As discussed in #324, we should write up a contribution guideline to make it easier to contribute and review.

Rules that are enforced by test cases are marked with an asterisk * and #328 will add the initial set of tests.

Updated 2018-02-04 by Andreas:

Coding Conventions

Match the existing code style, we generally stick to "Visual Studio defaults" and .NET Foundation Coding Guidelines
If you use ReSharper there is a settings file that will take effect automatically
There is an .editorconfig to configure whitespace and some C# syntax
Add the file header to new files you create

Test Code

Test class: Use Tests suffix for the type you are testing, such as UnitSystemTests
Test method: <method>_<condition>_<result> (Parse_AmbiguousUnits_ThrowsException)
If there are many tests for a single method, you can wrap those in an inner class named the same as the method and then you can skip that part of the test method names

Unit definitions (.JSON)

For a fairly complete summary of the unit definition JSON schema, see Meter of Length. It has prefix units and multiple cultures.

Conversion functions

Converting from unit A to B is achieved by first converting from unit A to the base unit, then from the base unit to unit B. To achieve this, each unit defines two conversion functions.

Prefer multiplication for FromUnitToBaseFunc (x*2.54e-2 for Inch to Meter)
Prefer division for FromBaseToUnitFunc (x/2.54e-2 for Meter to Inch)
Prefer scientific notation 1e3 and 1e-5 instead of 1000 and 0.00001
Prefer a constant if the conversion factor is finite (x*2.54e-2 for Inch)
Prefer a calculation if the conversion factor is infinite ((x/72.27)*2.54e-2 for PrinterPoint)

Units

Generally we try to name the units as what is the most widely used.

Use prefix for country variants, such as ImperialGallon and UsGallon

Note: We should really consider switching variant prefix to suffix, since that plays better with kilo, mega etc.. Currently we have units named KilousGallon and KiloimperialGallon, these would be better named KilogallonUs and KilogallonImperial.

Unit abbreviations

A unit can have multiple abbreviations per culture/language, the first one is used by ToString() while all of them are used by Parse().

Prefer the most widely used abbreviation in the domain, but try to adapt to our conventions
Add other popular variants to be able to parse those too, but take care to avoid abbreviation conflicts of units of the same quantity
Use superscript (cm², m³) instead of cm^2, m^3
Use ∆ for delta (not ▲)
Use · for products (N·m instead of Nm, N*m or N.m)
Prefer / over ⁻¹, such as km/h and J/(mol·K)
Use h for hours, min for minutes and s for seconds (m is ambiguous with meters)
Use suffixes to distinguish variants of similar units, such as gal (U.S.) vs gal (imp.) for gallons
- (U.S.) for United States
- (imp.) for imperial / British units

angularsen · 2017-11-24T19:41:07Z

I guess the obvious one is to at least link to here:
https://github.com/angularsen/UnitsNet/wiki/Adding-a-New-Unit

There is a (rather small) section on Code Style there, as well as some bullet points on conventions for JSON functions and test code when adding new units.

A couple of other things I can think of:

Add the library's file header to new files you create
Test method naming convention (it varies today)
Special symbols:
- ∆ for delta
- Use superscript (cm², m³)

Anything else? What inconsistencies did you notice in the existing code?

Usually people follow the existing code style fairly well and when adding new units it's very little new code to add, so it's not been a big issue fortunately.

jnyrup · 2017-11-25T08:02:19Z

We could add an [.editconfig](http://editorconfig.org/) to help having a consistent code style.
It is supported on numerous editors and can have different code style for .cs and .json files.

Regarding superscripts, we could test it.
If it were to be pretty, it would be an analyzer that generated a compiler error, if a superscript was not used.
The easy way to get the job done is to make a unit test that checks that no abbreviation in UnitSystem matches '\D\d'. Currently that catches AreaMomentOfInertiaUnit having both cm⁴ and cm^4 as abbreviations.

angularsen · 2017-11-25T12:03:13Z

Good idea, a .editorconfig file would be helpful. I propose we use or adapt Roslyn's file: https://github.com/dotnet/roslyn/blob/master/.editorconfig

A unit test to catch missing superscript, wrong delta symbol etc could be useful and should be easy enough to write, although hopefully we do catch these things in PR review when the guideline is set.

jnyrup · 2017-11-25T14:12:58Z

With good enough reviews unit tests are never necessary, but I wouldn't rely on humans being omniscient reviewers.

angularsen · 2017-11-26T12:05:59Z

Fair point!

jnyrup · 2017-11-26T20:45:20Z

There should probably also be some conventions about notation when writing abbreviations:
We have two different notation is used when the denominator is a product:

MolarEntropyUnit.JoulePerMoleKelvin
    J/(mol*K)

SpecificEntropyUnit.CaloriePerGramKelvin
    cal/g.K

Some units have different capitalisation across abbreviations:

British thermal unit is abbreviated BTU and Btu.
hour is abbreviated hr, Hr and h
second is abbreviated S and s

Some units are in plural while others aren't.

Flow has e.g FlowUnit.CentilitersPerMinute
Molarity has e.g. MolarityUnit.CentimolesPerLiter

When prefixing a unit with e.g kilo the unit should be lower case:

ForceUnit.KiloPond
BrakeSpecificFuelConsumptionUnit.GramPerKiloWattHour

Check that Milli is not spelled Mili

MassMomentOfInertiaUnit.KilotonneSquareMilimeter
MassMomentOfInertiaUnit.MegatonneSquareMilimeter
MassMomentOfInertiaUnit.TonneSquareMilimeter


        Changed ▲to ∆ as per angularsen#327 and angularsen#328


        Removed Delta suffix of TemperatureDelta units (#339)

Fixes #331 * Removed Delta suffix of TemperatureDelta units Fixed generate-code.bat to allow spaces in script path. * Fixed breaking tests caused by enums no longer matching by value with the addition of new non-"Delta" units * Changed BaseUnit from KelvinDelta to Kelvin and updated test code * Changed ▲to ∆ as per #327 and #328

gojanpaolo · 2018-01-11T19:17:29Z

Should there also be a convention when describing Area and Volume in other units (i.e. if the power should be used as a prefix or suffix of the unit)?
This is brought up with regards to this PR #364 to add AreaDensity and ElectricChargeDensity. Both units used the power as a prefix (KilogramsPerMeterSquared and CoulombPerMeterCube).

gojanpaolo · 2018-01-11T19:50:51Z

Convention for country suffix of units?
e.g. USSurveyFeet

ft (US)
ft (U.S.)
...

See also PR #361 (comment)

angularsen · 2018-01-13T20:47:18Z

@gojanpaolo The convention is currently ft (U.S.) for US units and th (imp.) for imperial/British units. Generally we try to follow the convention in the domain the units are used, so I would rather match the expectations of some people working with these units than being strictly consistent.

angularsen · 2018-01-13T20:54:18Z

@jnyrup This comment #328 (comment) was excellent, good catch finding all those discrepancies and variants.

To answer some of them:

notation is used when the denominator is a product

I prefer this form: J/(mol·K)

British thermal unit is abbreviated BTU and Btu

I prefer BTU

hour is abbreviated hr, Hr and h

I prefer h, but we do use min to disambiguate from meters

second is abbreviated S and s

We should use s, uppercase S is used for things like samples per second S/s

Agree with all your other points.
If you agree with my preferences above, then I propose we just create an issue to address these and fix them.

jnyrup · 2018-01-13T21:18:06Z

@angularsen
I agree with your preferences.
I have no experience with BTU/Btu and https://en.wikipedia.org/wiki/British_thermal_unit doesn't seem consistent?


        Create CONTRIBUTING.md

This is the first draft based on #328.

angularsen · 2018-02-04T19:15:05Z

I've assembled the first draft of the contribution guideline and put it both at the top of this issue as well as in a CONTRIBUTING file.

I'd like some input on this initial version so we can close this issue, then we can polish it and add more to it later.

angularsen · 2018-02-17T12:10:40Z

Closing this as we now have a contributing file up and running. Let's create a new issue if we want changes to it.

ferittuncer added the discuss label Nov 24, 2017

angularsen mentioned this issue Nov 25, 2017

Feature: Add LapseRate extensions #324

Merged

This was referenced Nov 29, 2017

Read units with UTF8 encoding #326

Merged

Acceleration units g, ft/s² and in/s² have been added. #333

Merged

angularsen mentioned this issue Dec 10, 2017

Removed Delta suffix of TemperatureDelta units (Fixes #331). #339

Merged

dutts added a commit to dutts/UnitsNet that referenced this issue Dec 10, 2017

Changed ▲to ∆ as per angularsen#327 and angularsen#328

acb2066

This was referenced Jan 13, 2018

Add .editorconfig to ensure consistent formatting #373

Closed

Add tests to ensure PR follows contribution guideline #374

Open

angularsen mentioned this issue Jan 17, 2018

Added physical quantity: BitRate #383

Merged

angularsen mentioned this issue Feb 3, 2018

Add Area SquareUsSurveyFeet #398

Merged

angularsen closed this Feb 17, 2018

angularsen / UnitsNet

Contribution Guideline #328

Contribution Guideline #328

ferittuncer commented Nov 24, 2017 •

edited by angularsen

angularsen commented Nov 24, 2017 •

edited

jnyrup commented Nov 25, 2017

angularsen commented Nov 25, 2017 •

edited

jnyrup commented Nov 25, 2017

angularsen commented Nov 26, 2017

jnyrup commented Nov 26, 2017 •

edited

gojanpaolo commented Jan 11, 2018

gojanpaolo commented Jan 11, 2018

angularsen commented Jan 13, 2018

angularsen commented Jan 13, 2018 •

edited

jnyrup commented Jan 13, 2018

angularsen commented Feb 4, 2018

angularsen commented Feb 17, 2018

angularsen / UnitsNet

Join GitHub today

Contribution Guideline #328

Contribution Guideline #328

Comments

ferittuncer commented Nov 24, 2017 • edited by angularsen

Coding Conventions

Test Code

Unit definitions (.JSON)

Conversion functions

Units

Unit abbreviations

angularsen commented Nov 24, 2017 • edited

jnyrup commented Nov 25, 2017

angularsen commented Nov 25, 2017 • edited

jnyrup commented Nov 25, 2017

angularsen commented Nov 26, 2017

jnyrup commented Nov 26, 2017 • edited

gojanpaolo commented Jan 11, 2018

gojanpaolo commented Jan 11, 2018

angularsen commented Jan 13, 2018

angularsen commented Jan 13, 2018 • edited

jnyrup commented Jan 13, 2018

angularsen commented Feb 4, 2018

angularsen commented Feb 17, 2018

ferittuncer commented Nov 24, 2017 •

edited by angularsen

angularsen commented Nov 24, 2017 •

edited

angularsen commented Nov 25, 2017 •

edited

jnyrup commented Nov 26, 2017 •

edited

angularsen commented Jan 13, 2018 •

edited