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.

Sign up for GitHub

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

don't parse multiple degree symbols and be more flexible in whitespace when parsing temperature #190

Merged
merged 6 commits into from
Feb 8, 2017
Merged

don't parse multiple degree symbols and be more flexible in whitespace when parsing temperature #190

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
08735c1
don't parse multiple degree symbols and be more flexible in whitespac…
derekmorr Feb 8, 2017
a966935
remove duplicate temperature parsing code
derekmorr Feb 8, 2017
6fa21b6
Merge branch 'master' into temperature-regex
derekmorr Feb 8, 2017
63369fe
Merge branch 'master' into temperature-regex
cquiroz Feb 8, 2017
6687a57
Merge branch 'master' of github.com:typelevel/squants into temperatur…
derekmorr Feb 8, 2017
6affa29
standardize syntax in the temperature regex
derekmorr Feb 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 8 additions & 14 deletions shared/src/main/scala/squants/thermal/Temperature.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

@leifwickland leifwickland Feb 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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})

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok this is addressed in the latest version.

s match {
case regex(value, Fahrenheit.symbol) ⇒ Success(Fahrenheit(value.toDouble))
case regex(value, "f") ⇒ Success(Fahrenheit(value.toDouble))
case regex(value, "F") ⇒ Success(Fahrenheit(value.toDouble))
case regex(value, Celsius.symbol) ⇒ Success(Celsius(value.toDouble))
case regex(value, "c") ⇒ Success(Celsius(value.toDouble))
case regex(value, "C") ⇒ Success(Celsius(value.toDouble))
case regex(value, Kelvin.symbol) ⇒ Success(Kelvin(value.toDouble))
case regex(value, "k") ⇒ Success(Kelvin(value.toDouble))
case regex(value, "K") ⇒ Success(Kelvin(value.toDouble))
case regex(value, Rankine.symbol) ⇒ Success(Rankine(value.toDouble))
case regex(value, "r") ⇒ Success(Rankine(value.toDouble))
case regex(value, "R") ⇒ Success(Rankine(value.toDouble))
case _ ⇒ Failure(QuantityParseException("Unable to parse Temperature", s))
case regex(value, unit) => unit match {
case "f" | "F" => Success(Fahrenheit(value.toDouble))
case "c" | "C" => Success(Celsius(value.toDouble))
case "k" | "K" => Success(Kelvin(value.toDouble))
case "r" | "R" => Success(Rankine(value.toDouble))
}
case _ => Failure(QuantityParseException("Unable to parse Temperature", s))
}
}

Expand Down
12 changes: 12 additions & 0 deletions shared/src/test/scala/squants/thermal/TemperatureSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,18 @@ class TemperatureSpec extends FlatSpec with Matchers {
Temperature("ZZ F").failed.get should be(QuantityParseException("Unable to parse Temperature", "ZZ F"))
}

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))
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Excellent 👍

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))
Temperature("10.22°f").get should be (Fahrenheit(10.22))

Temperature("10.22°°f").failed.get should be (QuantityParseException("Unable to parse Temperature", "10.22°°f"))
}

they should "properly convert to all supported Units of Measure (Scale)" in {
val x = Kelvin(0)
x.toKelvinScale should be(0)
Expand Down