Skip to content

Enhanced parsing of timestamp_utc #76

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

Merged
merged 4 commits into from
Dec 10, 2021
Merged

Enhanced parsing of timestamp_utc #76

merged 4 commits into from
Dec 10, 2021

Conversation

mrdalgaard
Copy link
Contributor

@mrdalgaard mrdalgaard commented Dec 8, 2021

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
Copy link
Contributor Author

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

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

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. :-)

@mrdalgaard
Copy link
Contributor Author

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.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

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 tannewt merged commit 98d505a into adafruit:main Dec 10, 2021
@mrdalgaard
Copy link
Contributor Author

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 ! :-)

adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Dec 14, 2021
Updating https://github.com/adafruit/Adafruit_CircuitPython_GPS to 3.9.6 from 3.9.5:
  > Merge pull request adafruit/Adafruit_CircuitPython_GPS#76 from mrdalgaard/main
  > update rtd py version

Updating https://github.com/adafruit/Adafruit_CircuitPython_PyPortal to 6.1.0 from 6.0.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_PyPortal#116 from joebaird/main
  > update rtd py version

Updating https://github.com/adafruit/Adafruit_CircuitPython_Seesaw to 1.10.4 from 1.10.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_seesaw#90 from todbot/main
  > update rtd py version

Updating https://github.com/adafruit/Adafruit_CircuitPython_Display_Shapes to 2.4.1 from 2.4.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_Display_Shapes#45 from dhalbert/make-package

Updating https://github.com/adafruit/Adafruit_CircuitPython_RTTTL to 2.4.10 from 2.4.9:
  > Merge pull request adafruit/Adafruit_CircuitPython_RTTTL#29 from FoamyGuy/pyportal_fix

Updating https://github.com/adafruit/Adafruit_CircuitPython_TinyLoRa to 2.2.5 from 2.2.4:
  > update rtd py version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Usage of float causes timestamp_utc to be imprecise
2 participants