dotnet / iot

This repo includes .NET Core implementations for various IoT boards, chips, displays and PCBs.
MIT License
2.18k stars 586 forks source link

String formatting of Temperature, Pressure and future physical quantities #1047

Closed pgrawehr closed 4 years ago

pgrawehr commented 4 years ago

While working on the WeatherHelper stuff, I found that it was pretty inconvenient that the Temperature struct did not have a ToString() member. So I added one, and also added a public string ToString(string formatArgs, IFormatProvider culture) method, that takes formatting arguments to print Temperature in different units.

With PR #1046 it is now proposed to do the same for Pressure, but there are a lot of possible ways one may want to format such values, so we need a general concept on how to create such formatting members and what kind of formatting options should be supported.

As a reference, the two proposed ToString() functions for Temperature and Pressure:


        /// <summary>
        /// Returns the string representation of this pressure, with the given format string and using the given culture.
        /// Valid format specifiers are:
        /// PA: Pascal
        /// MBAR: Millibar
        /// KPA: Kilopascal
        /// HPA: Hectopascal
        /// INHG: Inch of mercury
        /// MMHG: Millimeter of mercury
        /// An extra number defines the number of decimal digits to use (default 1)
        /// <example>
        /// <code>
        /// var s = p.ToString("PA2"); -> "101325.01 Pa"
        /// var s = p.ToString("HPA2"); --> "1013.25 hPa"
        /// </code>
        /// </example>
        /// </summary>
        /// <param name="formatArgs">Format string</param>
        /// <param name="culture">Culture to use. Affects the format of the number.</param>
        /// <returns>String representation</returns>
        public string ToString(string formatArgs, IFormatProvider culture)

        /// <summary>
        /// Returns the string representation of this temperature, with the given format string and using the given culture.
        /// Valid format specifiers are:
        /// C: Degrees celsius
        /// F: Degrees Fahrenheit
        /// K: Degrees Kelvin
        /// An extra number defines the number of decimal digits to use (default 1)
        /// <example>
        /// <code>
        /// var s = t.ToString("K2"); -> "293.36°K"
        /// var s = t.ToString("C"); --> "20.21°C"
        /// </code>
        /// </example>
        /// </summary>
        /// <param name="formatArgs">Format string</param>
        /// <param name="culture">Culture to use. Affects the format of the number.</param>
        /// <returns>String representation</returns>
        public string ToString(string formatArgs, IFormatProvider culture)

Questions include:

I guess that including a standard formatting for the parameterless ToString() is less disputed and should be done in either case.

CC: @MarkCiliaVincenti @krwq

krwq commented 4 years ago

cc: @tarekgh

krwq commented 4 years ago

Some open questions:

krwq commented 4 years ago

Also:

krwq commented 4 years ago

If possible we should reuse NumberFormatInfo here, perhaps this should work very similar to currency formatting (per @tarekgh offline suggestion): https://docs.microsoft.com/en-us/dotnet/standard/base-types/standard-numeric-format-strings#the-currency-c-format-specifier

Note we can also propose a feature in NumberFormatInfo if we can't re-use it right now

tarekgh commented 4 years ago

I support taking advantage of NumberFormatInfo even it lacks support for temperature and Pressure which we may add later to the framework. but at least we can use it for format the number (using the separators and decimal points) and we always append the unit at the end for now.

MarkCiliaVincenti commented 4 years ago

I'm actually proud I opened a can of worms :) I will give my feedback later.

pgrawehr commented 4 years ago

If internally using ToString("F"), NumberFormatInfo is being used implicitly. The implementation already properly decides between . and , for instance.

tarekgh commented 4 years ago

If internally using ToString("F"), NumberFormatInfo is being used implicitly. The implementation already properly decides between . and , for instance.

Yes, you are right. I was hoping not introducing special format inside Temperature and Pressure types and have the formatting standardized in the cultures (in NumberFormatInfo) as we do for currency and other numbers.

krwq commented 4 years ago

@tarekgh there are some unit multipliers which might be problematic without being a bit more explicit, i.e. Hectopascal vs Pascal - depending on the usage you might or not want the multiplier but inch vs mm of hg/K or C on the other hand is something which I agree can be related to culture

tarekgh commented 4 years ago

there are some unit multipliers which might be problematic without being a bit more explicit, i.e. Hectopascal vs Pascal - depending on the usage you might or not want the multiplier but inch vs mm of hg/K or C on the other hand is something which I agree can be related to culture

The unit shouldn't specified in the format :-) if cultures support temperature and pressure units, then users can pass the culture to get the format for the matched units as desired. think about it as how we handle currency today.

pgrawehr commented 4 years ago

What krwq means: For some quantities, the default unit is clear. For temperature, it's Fahrenheit in the US and Celsius everywhere else. But for others it's not so clear: For pressure, it depends on context whether you want mmHG/inchHG or Bar/Pascal/Hectopascal. Or for speed, it might normally be Kilometers/hour vs Miles/hour, except if you are into aviatics or shipping, where it would be knots. And for physicans, it's meters/second (which is the SI unit for it).

pgrawehr commented 4 years ago

Related: #869 and the question, which units (or actually more precise: which physical quantities) shall have their own class representation.

krwq commented 4 years ago

Marking as vNext. At minimum we should figure out if we want to pull out whatever has already been added or not.

MarkCiliaVincenti commented 4 years ago

One thing to keep in mind is that once these are finalized we should update all the samples.

MarkCiliaVincenti commented 4 years ago

Also, if we're going to do these, we might want to discuss creating a new unit for distance, which can be used as the return type of WeatherHelper.CalculateAltitude, for example. Currently, it returns meters, but maybe someone wants it in feet or yards.

I'm surprised Microsoft doesn't have a standalone library for these units which could be used by other libraries. They seem almost out of scope in IoT.

krwq commented 4 years ago

I'd start with designing formatters but have other units in mind when designing instead of adding them in one go.

Note: to move this forward we need exact proposal of how formatters will work including plan for future units. We need to start with investigating NumberFormatInfo and if can it be currently extended and if not how can we make it extendable. Honestly unless it's already possible to extend it I suspect we won't add these formatters in time for vNext since we won't be able to use the APIs until we ship (although we can hack it or partially re-implement as a temporary solution)

pgrawehr commented 4 years ago

Note: I believe we should be consistently talking of "(physical) quantities" when we talk about Temperature, Pressure, Distance or Speed, and Units if we talk about meters, degrees or Miles per Hour. This reduces confusion. Each quantity can be represented by different units, but a specific unit may in practical applications represent different quantities (i.e. in aviatics, the default unit for the horizontal distance is nautical miles, while for the vertical distance it is feet).

That said, I think we should have support for at least the following quantities:

krwq commented 4 years ago

perhaps we should refrain with this issue for now (and revert all formatters for now) and focus on adding more units. Once we got those then we can focus on formatters and other utilities (perhaps for conversion or some operators to convert between units)

pgrawehr commented 4 years ago

I've got some of those prepared already, I can provide a PR maybe tomorrow. Otherwise, I'm fine with removing that for now. Would you remove the standard ToString() as well?

krwq commented 4 years ago

@pgrawehr cool! Please do PR per unit though. For now I'd start with returning anything in ToString although I think we should put some kind of comment in the xml doc that the behavior may change in the future

MarkCiliaVincenti commented 4 years ago

Does it make sense to have them in the IoT library though, or should we have them elsewhere (ideally in a .NET Standard library)? For example I was working on a completely separate project and wanted to use the Temperature unit.

pgrawehr commented 4 years ago

@MarkCiliaVincenti You might be right here. With the ease of creating (and using) new nuget packages, packages should be small. So this should probably go into its own package (i.e. System.Physics) or so. This doesn't necessarily mean we can't keep it in this repo, though (or even that this couldn't be moved/refactored later). But it might be good not to use the Iot.Devices namespace for these. I do like System.Physics, since that would keep things open for stuff like physical calculations (based on those objects) etc.

JTrotta commented 4 years ago

Have you ever used this library UnitsNet ? It's complete and works very well, I used it for years, instead of reinventing the wheel. It's just a suggestion.

pgrawehr commented 4 years ago

Well, I didn't know this, but it does look exactly like what we need. It even has operators to compute i.e. Speed out of distance and time. The license is also matching. And it has formatting members, too 👍

krwq commented 4 years ago

@JTrotta @pgrawehr I'm not 100% sure - I'd personally like that but there are also questions about servicing in case we ever have a security bug which is in that library. There are some protocols we have to follow as Microsoft which is hard to enforce on external packages. I will need to ask around. @joperezr any clues?

krwq commented 4 years ago

Let's move conversation about units to https://github.com/dotnet/iot/issues/869

pgrawehr commented 4 years ago

Closing this, as the move to UnitsNet removes the burden of the formatting problem from our library. Opened https://github.com/angularsen/UnitsNet/issues/786 and https://github.com/angularsen/UnitsNet/issues/787 for some findings over there (maybe more to come, as we start using it).