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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add and use time related constants #32065

Merged
merged 16 commits into from Feb 23, 2020
Merged

Conversation

@springstan
Copy link
Member

springstan commented Feb 21, 2020

Breaking change

Time units have been standardized throughout all integrations.
Therefore, if you were using a non standard one so far please change it to the following:

  • 渭s for microseconds
  • ms for milliseconds
  • s for seconds
  • min for minutes
  • h for hours
  • d for days
  • w for weeks
  • m for months
  • y for years

Proposed change

Follow up PR to #31781

Adding some constants that are time related to be consistent in the unit of measurement.

CC @scop

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to PR: #31781
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 馃 Silver
  • 馃 Gold
  • 馃弳 Platinum
@springstan springstan requested review from cgtobi, dgomes, fabaff, Quentame, robbiet480, scop and home-assistant/core as code owners Feb 21, 2020
@project-bot project-bot bot added this to Needs review in Dev Feb 21, 2020
springstan added 3 commits Feb 21, 2020
@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 21, 2020

Codecov Report

Merging #32065 into dev will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #32065      +/-   ##
==========================================
+ Coverage   94.71%   94.72%   +0.01%     
==========================================
  Files         766      767       +1     
  Lines       55568    55483      -85     
==========================================
- Hits        52630    52557      -73     
+ Misses       2938     2926      -12     
Impacted Files Coverage 螖
homeassistant/components/zha/lock.py 88.33% <0.00%> (-2.21%) 猬囷笍
homeassistant/components/ring/config_flow.py 76.19% <0.00%> (-2.08%) 猬囷笍
homeassistant/components/zha/switch.py 94.33% <0.00%> (-1.19%) 猬囷笍
homeassistant/components/zha/cover.py 92.94% <0.00%> (-1.00%) 猬囷笍
homeassistant/components/zha/binary_sensor.py 95.45% <0.00%> (-0.80%) 猬囷笍
homeassistant/components/zha/fan.py 96.92% <0.00%> (-0.55%) 猬囷笍
homeassistant/components/zha/device_tracker.py 97.95% <0.00%> (-0.46%) 猬囷笍
homeassistant/components/dynalite/const.py 100.00% <0.00%> (酶) 猬嗭笍
homeassistant/components/zha/device_action.py 100.00% <0.00%> (酶) 猬嗭笍
homeassistant/components/asuswrt/device_tracker.py 65.38% <0.00%> (酶)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update dd8597c...97e4e8f. Read the comment docs.

homeassistant/const.py Show resolved Hide resolved
homeassistant/components/raincloud/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/apcupsd/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/opentherm_gw/const.py Outdated Show resolved Hide resolved
Dev automation moved this from Needs review to Review in progress Feb 21, 2020
Copy link
Member

rohankapoorcom left a comment

Overall, looks good to me. I think there's one outstanding change that @scop already requested, otherwise this seems ready.

Regarding marking it as a breaking change, I think it's worth writing out something indicating that all of the time units have been standardized, providing the list of standardized units and saying that if you were using a non standard one in an automation or sensor or something, it has now been changed.

Do we have a linter or anything that can make sure that we caught all occurrences of these and converted them to constants?

@scop

This comment has been minimized.

Copy link
Contributor

scop commented Feb 22, 2020

Would be great if we'd have a tool to help against magic strings creeping in in the future. That'd be a difficult task to do robustly, though.

I happened to have a local in-progress branch for the same change, I'll do a quick diff to see if there were any.

BTW, JFYI, I have a WIP branch that addresses additional area, mass, concentration, speed, volume, and some other units here and there. I'll rebase once this in and file a PR.

Copy link
Contributor

scop left a comment

Coverage was better than in my local branch, just one bug spotted when doing the diff.

homeassistant/components/isy994/sensor.py Outdated Show resolved Hide resolved
@springstan

This comment has been minimized.

Copy link
Member Author

springstan commented Feb 22, 2020

BTW, JFYI, I have a WIP branch that addresses additional area, mass, concentration, speed, volume, and some other units here and there. I'll rebase once this in and file a PR.

Sounds great! FYI I have opened another PR #32094 for the percentage sign.

@springstan

This comment has been minimized.

Copy link
Member Author

springstan commented Feb 22, 2020

@rohankapoorcom FYI I have added the breaking change paragraph to the PR.

@scop
scop approved these changes Feb 23, 2020
Dev automation moved this from Review in progress to Reviewer approved Feb 23, 2020
Copy link
Contributor

scop left a comment

Found a few more to look into while at it. Marked in inline comments where possible, plus this one:

tests/components/integration/test_sensor.py:184:            "unit_time": "s",
homeassistant/components/apcupsd/sensor.py Outdated Show resolved Hide resolved
Dev automation moved this from Reviewer approved to Review in progress Feb 23, 2020
springstan added 2 commits Feb 23, 2020
@springstan

This comment has been minimized.

Copy link
Member Author

springstan commented Feb 23, 2020

@scop thanks for reviewing this PR again, I have done another search through the entire project and replaced the rest :)

@scop
scop approved these changes Feb 23, 2020
Copy link
Contributor

scop left a comment

Yeah, I don't know the isy994 stuff either and there are no comments citing a source that I can see, but assuming they're correct, LGTM.

Dev automation moved this from Review in progress to Reviewer approved Feb 23, 2020
@scop scop merged commit a85808e into home-assistant:dev Feb 23, 2020
10 checks passed
10 checks passed
CI Build #20200223.29 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch 94.73% of diff hit (target 94.71%)
Details
codecov/project 94.72% (target 90.00%)
Details
Dev automation moved this from Reviewer approved to Done Feb 23, 2020
@springstan

This comment has been minimized.

Copy link
Member Author

springstan commented Feb 23, 2020

@scop thanks 馃憤

@scop scop mentioned this pull request Feb 23, 2020
5 of 20 tasks complete
@lock lock bot locked and limited conversation to collaborators Feb 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can鈥檛 perform that action at this time.