adafruit / Adafruit_CircuitPython_GPS

GPS parsing module for CircuitPython. Meant to parse NMEA data from serial GPS modules.
MIT License
75 stars 58 forks source link

Enhanced parsing of timestamp_utc #76

Closed mrdalgaard closed 2 years ago

mrdalgaard commented 2 years ago

Closes #75

Uses string parsing instead of math for generating timestamp_utc. Avoids imprecise usage of float.

Sorry if i messed this up, its my first pull request.

Tested on Adafruit Feather Bluefruit Sense, and CP7.2.0-alpha.0. I do not have any low-memory MCUs to test on such as the M0

mrdalgaard commented 2 years ago

I guess the tests are failing because of the changed type. Before trying to change these, some feedback would be appreciated.

mrdalgaard commented 2 years ago

The fix itself looks good to me. Since this library has pretty good tests, I think you should add a new test for the specific date you found failed with the float code.

So, copy one of the test functions that you had to modify to use a string and make it use the date/time that fails with floats. This will prevent anyone from changing it back to float because it won't pass. :-)

The tests actually already fails when trying with floats or ints:

>>> gps._update_timestamp_utc(122345)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 5, in _update_timestamp_utc
TypeError: 'int' object isn't subscriptable
>>> gps._update_timestamp_utc(float(122345))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 5, in _update_timestamp_utc
TypeError: 'float' object isn't subscriptable
>>> gps._update_timestamp_utc("122345", date=343434)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 16, in _update_timestamp_utc
TypeError: 'int' object isn't subscriptable
>>> gps._update_timestamp_utc("122345")
>>>

Because you can't slice ints and floats.

mrdalgaard commented 2 years ago

I see your point. What I should have said was that when bugs like this are found, it's good practice to add a new test that fails before fixing it. Then, you know you fixed the issue.

However, this may not work since it is a MicroPython VM bug, not a Python one. So, I'll approve and merge. Thanks!

@tannewt Thank you for explaining. I'm not familiar with best pactice for writing tests, but i will keep your advice in mind for next time ! :-)