-
Notifications
You must be signed in to change notification settings - Fork 7
Refactor WeatherSource and WeatherSourceWrapper. #724
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
Refactor WeatherSource and WeatherSourceWrapper. #724
Conversation
# Conflicts: # CHANGELOG.md # src/main/scala/edu/ie3/simona/config/ConfigFailFast.scala # src/main/scala/edu/ie3/simona/service/weather/WeatherSource.scala # src/main/scala/edu/ie3/simona/service/weather/WeatherSourceWrapper.scala # src/test/scala/edu/ie3/simona/config/ConfigFailFastSpec.scala # src/test/scala/edu/ie3/simona/service/weather/WeatherSourceSpec.scala
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
…-Wrapper # Conflicts: # src/main/scala/edu/ie3/simona/config/ConfigFailFast.scala
…-Wrapper # Conflicts: # CHANGELOG.md # src/main/scala/edu/ie3/simona/service/weather/SampleWeatherSource.scala # src/test/scala/edu/ie3/simona/service/weather/WeatherSourceSpec.scala
…-Wrapper # Conflicts: # src/test/scala/edu/ie3/simona/config/ConfigFailFastSpec.scala
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for make this more comprehensive
|
@danielfeismann I was still reviewing. Will just submit the points I found now after the fact |
| s"No coordinate source defined! This is currently not supported! Please provide the config parameters for one " + | ||
| s"of the following coordinate sources:\n\t${supportedCoordinateSources.mkString("\n\t")}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should still be possible to run simulations with no weather data at all. Did you check if that works? We only need to fail here if there's a weather source but no coordinate source
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, there has been no option to not use weather data before. So this would be a larger issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sebastian-peter Will you create a new issue for this?
| case None | Some(_) => | ||
| throw new InvalidConfigParameterException( | ||
| s"No weather source defined! This is currently not supported! Please provide the config parameters for one " + | ||
| s"of the following weather sources:\n\t${supportedWeatherSources.mkString("\n\t")}" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar issue here
|
Sorry @sebastian-peter wasn't aware that you also had a look |
Resolves #180
May help with or resolves #625