Skip to content

Add clarifying comment about unit of elevation#88489

Merged
balloob merged 2 commits into
devfrom
elevation_meters
Feb 22, 2023
Merged

Add clarifying comment about unit of elevation#88489
balloob merged 2 commits into
devfrom
elevation_meters

Conversation

@emontnemery
Copy link
Copy Markdown
Contributor

@emontnemery emontnemery commented Feb 20, 2023 •
edited
Loading

Proposed change

SSIA

Related issue: #88283

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)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • 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.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@emontnemery emontnemery requested a review from a team as a code owner February 20, 2023 13:22
epenet
epenet reviewed Feb 20, 2023
View reviewed changes
Comment thread homeassistant/core.py Outdated

self.latitude: float = 0
self.longitude: float = 0
# Elevation (always in meters regardless of the unit system)
Copy link
Copy Markdown
Contributor

@epenet epenet Feb 20, 2023

Choose a reason for hiding this comment

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

Can/Should this be a docstring?

frenck
frenck requested changes Feb 21, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Good point by epenet on making this a docstring

@home-assistant
Copy link
Copy Markdown
Contributor

home-assistant Bot commented Feb 21, 2023

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant Bot marked this pull request as draft February 21, 2023 10:31
@emontnemery
Copy link
Copy Markdown
Contributor Author

emontnemery commented Feb 22, 2023 •
edited
Loading

The docstring is indeed helpful in the IDE.
Should other commented members also have their comments changed to docstrings? In this PR or in a follow-up?

core/homeassistant/core.py

Lines 1831 to 1835 in 6595ce3

# If True, pip install is skipped for requirements on startup
self.skip_pip: bool = False
# List of packages to skip when installing requirements on startup
self.skip_pip_packages: list[str] = []

@emontnemery emontnemery marked this pull request as ready for review February 22, 2023 11:53
@home-assistant home-assistant Bot requested a review from frenck February 22, 2023 11:53
@balloob balloob merged commit 594b7b7 into dev Feb 22, 2023
@balloob balloob deleted the elevation_meters branch February 22, 2023 14:06
raman325 added a commit to raman325/home-assistant that referenced this pull request Feb 22, 2023
@raman325
Merge branch 'dev' into target
5a6744c 
* dev: (60 commits)
  Update frontend to 20230222.0 (home-assistant#88615)
  Add controller support to `zwave_js/subscribe_firmware_update_status` (home-assistant#87348)
  Bump Freebox to 1.1.0 (home-assistant#88609)
  Always include platform in `config/entity_registry/list_for_display` (home-assistant#88601)
  Add dsk option to zwave_js/add_node WS command (home-assistant#87823)
  Update zwave_js FirmwareUploadView to support controller updates (home-assistant#87239)
  Add new zwave_js WS command to parse DSK from QR code (home-assistant#87237)
  Diagnostics report for Thread networks (home-assistant#88541)
  Set default for `hass_config_yaml` fixture to "" (home-assistant#88608)
  Bump reolink-aio to 0.5.0 (home-assistant#88594)
  Bump intents package version; hassil==1.0.5; home-assistant-intents==2023.2.22 (home-assistant#88605)
  Add Reolink update entity (home-assistant#87865)
  Fix cover template: optimistic mode is ignored (home-assistant#87925)
  Fix 500 error when getting calendar events (home-assistant#88276)
  Add clarifying comment about unit of elevation (home-assistant#88489)
  Add ZHA "consumer connected" binary sensor for Xiaomi EU plugs (home-assistant#88194)
  Bump Insteon dependencies (home-assistant#88514)
  Use load_json_object in ecobee (home-assistant#88584)
  Use load_json_object in html5 (home-assistant#88586)
  Improve type hint in homeassistant trigger (home-assistant#88596)
  ...
@github-actions github-actions Bot locked and limited conversation to collaborators Feb 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@epenet epenet epenet left review comments

@balloob balloob balloob approved these changes

@frenck frenck Awaiting requested review from frenck

Assignees

No one assigned

Labels

cla-signed code-quality core small-pr PRs with less than 30 lines.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@emontnemery @frenck @balloob @epenet