Skip to content
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

Replace datetime.utcnow + datetime.utcfromtimestamp #1809

Merged
merged 3 commits into from
Sep 14, 2023
Merged

Replace datetime.utcnow + datetime.utcfromtimestamp #1809

merged 3 commits into from
Sep 14, 2023

Conversation

cdce8p
Copy link
Contributor

@cdce8p cdce8p commented Aug 20, 2023

@codecov
Copy link

codecov bot commented Aug 20, 2023

Codecov Report

Merging #1809 (9bcd715) into master (2546b04) will decrease coverage by 0.02%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master    #1809      +/-   ##
==========================================
- Coverage   80.82%   80.81%   -0.02%     
==========================================
  Files         192      192              
  Lines       18541    18541              
  Branches     4014     4014              
==========================================
- Hits        14985    14983       -2     
- Misses       3278     3280       +2     
  Partials      278      278              
Files Changed Coverage Δ
miio/miot_cloud.py 75.64% <50.00%> (ø)
miio/miioprotocol.py 33.56% <100.00%> (ø)
miio/protocol.py 96.80% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -48,7 +48,7 @@ def __init__(

self._discovered = False
# these come from the device, but we initialize them here to make mypy happy
self._device_ts: datetime = datetime.utcnow()
self._device_ts: datetime = datetime.now(tz=timezone.utc).replace(tzinfo=None)
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the PR, @cdce8p! I'm wondering if it's necessary to have replace() calls at all? Alas, I'm not going to have access to any test devices until next month, so I cannot really test this myself.

The CI failures are irrelevant to this PR, so they are not blockers to get this merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't test it myself either, unfortunately.

However, I believe you might be correct here. The call in _valid_cache is covered by the test suite. As for the protocol: AFAICT the conversion happens inside the TimeAdapter. obj.timetuple() strips the timezone anyway and for the decode we might as well leave the tz in place.

@rytilahti rytilahti added this to the 0.6.0 milestone Aug 28, 2023
@cdce8p
Copy link
Contributor Author

cdce8p commented Sep 14, 2023

@rytilahti Gentle ping. Anything left todo here for me?

@rytilahti
Copy link
Owner

@cdce8p thanks for the ping and sorry for the delay. I think it's good to go, but I haven't been still able to test it on any real devices. But well, we can always revert it if needed, so I just updated on top of the current master in hopes it fixes the CI issues and I'll merge this when the checks pass.

@rytilahti rytilahti merged commit f7b554f into rytilahti:master Sep 14, 2023
12 of 18 checks passed
@cdce8p cdce8p deleted the replace-utcnow branch September 14, 2023 12:09
@cdce8p
Copy link
Contributor Author

cdce8p commented Sep 14, 2023

so I just updated on top of the current master in hopes it fixes the CI issues and I'll merge this when the checks pass.

FYI the windows error seems to be a poetry issue. Downgrading to poetry<1.4.0 might help.
python-poetry/poetry#7611

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.

2 participants