Fix zone offset parsing #40
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
spec/data/zone_spec.rb
Outdated
| end | ||
|
|
||
| it 'returns true when preceded by a space' do | ||
| expect('August 9, 6:56 AM -10').to be true |
There was a problem hiding this comment.
test is missing method to test on expectation.
expect( ZoneOffset.detect?('August 9, 6:56 AM -10') ).to be true
There was a problem hiding this comment.
Thanks, I had some unpushed fixes here. 😆
spec/data/zone_spec.rb
Outdated
| end | ||
|
|
||
| it 'returns false when part of a date' do | ||
| expect('2017-10-10').to be false |
There was a problem hiding this comment.
Missing ZoneOffset.detect?
Parse numeric strings to avoid errors when API responses don't send a known zone string but default to a numeric offset instead.
The entire time string is passed to the zone parser, so look for numeric offsets that may be preceded by a space.
Parsing the time string using `strptime` against the format string was causing errors. Unsure if all times sent from Wunderground are like this, or only some locations. In any case, both formats are recognized without the format string using the `DateTime.parse` method
avit
force-pushed
the
zone_offset_parsing
branch
from
34299c7 to
ed4ac03
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We were seeing errors from Weather Underground for certain locations where a time zone name was not being returned, and Barometer was unable to parse the offset value. It looks like the zone data parsing was tested under the assumption that it expects a numeric value, when in fact it receives a string value.
This change teaches the zone data to parse standard offset strings of
+HHor+HHMM.The original implementation was treating numeric values as hours, and this assumption still remains when passing integer types. This means
"1200"and1200are understood differently as 12 hours vs. 1200 hours, respectively.