don't parse multiple degree symbols and be more flexible in whitespace when parsing temperature #190
Conversation
…e when parsing temperature
|
A second commit removes duplicate temperature parsing code (and removed the extraneous degree matches, since the degree symbol isn't captured by the regex). |
| they should "be flexible in parsing strings with regard to degree symbol and whitespace" in { | ||
| Temperature("10.22 f").get should be (Fahrenheit(10.22)) | ||
| Temperature("10.22 °f").get should be (Fahrenheit(10.22)) | ||
| Temperature("10.22 °f").get should be (Fahrenheit(10.22)) |
There was a problem hiding this comment.
I guess this is ok though I can imagine somebody arguing that we should only allow one format. How is this for other units? do we allow e.g. m / s?
There was a problem hiding this comment.
We already allow this syntax with the current Temperature parsing regex. I was just adding tests to make sure I didn't break anything while I was changing the regex.
| @@ -141,21 +141,15 @@ object Temperature extends Dimension[Temperature] with BaseDimension { | |||
| def apply[A](n: A, scale: TemperatureScale)(implicit num: Numeric[A]) = new Temperature(num.toDouble(n), scale) | |||
|
|
|||
| def apply(s: String): Try[Temperature] = { | |||
| val regex = "([-+]?[0-9]*\\.?[0-9]+(?:[eE][-+]?[0-9]+)?)[ °]*(f|F|c|C|k|K|r|R)".r | |||
| val regex = "([-+]?[0-9]*\\.?[0-9]+(?:[eE][-+]?[0-9]+)?)*[ ]*°? *(f|F|c|C|k|K|r|R)".r | |||
There was a problem hiding this comment.
For consistency, I suggest one of the following:
- enclose space in square brackets both times, i.e.
[ ]*°?[ ]* - don't place space in square brackets, i.e.
*°? * - take it to eleven and allow all the goofy varieties of horizontal whitespace including tab and unicode, i.e.
[\\p{Zs}\\t]*°?[\\p{Zs}\\t]*
(See http://www.fileformat.info/info/unicode/category/Zs/list.htm for the meaning of \p{Zs})
There was a problem hiding this comment.
Ok this is addressed in the latest version.
|
Is this ready to be merged? |
|
Yes. |
This would fix #189. I adapted the regex slightly from the current pureconfig-squants version.
This disallows multiple degree symbols. Arguably this was a bug. If this PR is merged, we should add a note to the release notes that this is technically a breaking change (we're less liberal in what inputs we parse).
We should also look at the other string parsers to see if they're similarly affected.