-
Notifications
You must be signed in to change notification settings - Fork 18
Changed tz_offset to seconds, updated docstring #11
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
Conversation
@brentru - Apparently I'm not formatting the docstring correctly. Where can I find info on what the formatting rules are? (sorry for all the hassle for such a small change) |
@theelectricmayhem Looks like it's failing on Sphinx (https://github.com/adafruit/Adafruit_CircuitPython_NTP/pull/11/checks?check_run_id=668814279#step:14:1). Try adding a blank line at the end of your docstring:
We have some instructions about running sphinx locally here: https://learn.adafruit.com/creating-and-sharing-a-circuitpython-library/sharing-our-docs-on-readthedocs#sphinx-5-1, |
@brentru please do the full review :) |
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.
@theelectricmayhem I think keeping it as minutes is OK (http://man7.org/linux/man-pages/man2/settimeofday.2.html).
Is there any reason you're modifying it to use seconds?
@brentru I modified it to seconds (from hours) based on feedback from @jepler. Also, it seemed to make the most sense for making adjustments to, say I saw the error about "Field list ends without a blank line; unexpected unindent." but the previous version also did not have a trailing blank line and it did not fail the Sphinx test. Let me know if you'd prefer minutes rather than seconds and I'll make changes. |
Mercy! @brentru if you could take a look at this, I'd appreciate it. I can't, for the life of me find the offending formatting. |
@theelectricmayhem I'm not sure where the issue is, @sommersoft could you please take a look? Thanks! |
The documentation building process is finished. I think the remaining question is: minutes or seconds. UNIX seems inconsistent on the subject. Reasons to use minutes:
Reasons to use seconds:
Many newer systems store time zone offsets as general time intervals (similar to datetime.timedelta in CPython3) so they can represent arbitrary durations without the problem of knowing what the basic unit actually is. |
@jepler I think using seconds would be useful for this library, for now. We could move to time deltas in a future PR (I didn't know about them, thanks for mentioning). Are you OK with this? |
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.
Yes, I am happy with the current state of the pr
Updating https://github.com/adafruit/Adafruit_CircuitPython_RFM9x to 2.0.1 from 2.0.0: > Merge pull request adafruit/Adafruit_CircuitPython_RFM9x#45 from jerryneedell/jerryn_size Updating https://github.com/adafruit/Adafruit_CircuitPython_LED_Animation to 2.3.1 from 2.3.0: > Merge pull request adafruit/Adafruit_CircuitPython_LED_Animation#51 from cjsieh/customcolorchase > Merge pull request adafruit/Adafruit_CircuitPython_LED_Animation#52 from kattni/sequence-fix Updating https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT to 3.1.1 from 3.1.0: > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#41 from brentru/example-fix Updating https://github.com/adafruit/Adafruit_CircuitPython_NTP to 2.1.0 from 2.0.0: > Merge pull request adafruit/Adafruit_CircuitPython_NTP#11 from theelectricmayhem/patch-1
No description provided.