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

optional displaying the sensors location on the map #12375

Merged
merged 4 commits into from Mar 6, 2018

Conversation

Projects
None yet
4 participants
@c7h
Copy link
Contributor

commented Feb 13, 2018

Description:

the sensor is now using the correct attributes (latitued and longitued) which enables them to show up on the map.

example

Breaking change

The sensor attributes for GPS location used to be lat and long. This has been changed to latitude and longitude respectively.

Related issue (if applicable): fixes #

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4680

Example entry for configuration.yaml (if applicable):

Checklist:

  • The code change is tested and works locally.

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

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

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.
Changed sensor attributes for GPS
the sensor is now using the correct attributes (latitued and
longitued) which enables them to show up on the map.
@tinloaf
Copy link
Contributor

left a comment

Looks good! This is, however, a breaking change. Could you write a 1-2 sentences describing this in a special "Breaking Change" paragraph in your original PR text?

@c7h c7h closed this Feb 13, 2018

@c7h c7h reopened this Feb 13, 2018

@fabaff
Copy link
Member

left a comment

Showing a sensor on the map needs to be an optional feature because if the sensor isn't around the corner of your location the map will be loaded with a zoom level that everything fits.

We discussed this for the iss platform already deeply and agreed on using show_on_map.

@tinloaf

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2018

Makes sense. The "airvisual" sensor seems to use the same config option. Perhaps it would be beneficial to extract that into const.py.

added option to display the sensor on the map
the configuration will be extended by the optional 'show_on_map' flag.
Default is not display the sensor on the map.
@c7h

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2018

@fabaff I've implemented the requested change. It makes sense to give the user an option to exclude the sensor from the map. The documentation will require an update now as well.

@tinloaf

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2018

Now it technically has less features than before if CONF_SHOW_ON_MAP = False, because the coordinates are not part of the attributes anymore. The iss platform solves this by setting the lat and lon attributes if CONF_SHOW_ON_MAP = False, and the ATTR_LATITUDE and ATTR_LONGITUDE if CONF_SHOW_ON_MAP = True.

If you were to implement it in the same way, and set the default for CONF_SHOW_ON_MAP to false, it even wouldn't be a breaking change anymore.

@c7h

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2018

That's a valid point. I will change it accordingly. Not breaking things is good 👍

@c7h c7h changed the title changed luftdaten sensors location attributes optional displaying the sensors location on the map Feb 14, 2018

@tinloaf

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2018

Looks good now. 👍

@fabaff fabaff removed the breaking change label Feb 15, 2018

@@ -121,6 +126,9 @@ def device_state_attributes(self):
'lat': self.luftdaten.data.meta['latitude'],
'long': self.luftdaten.data.meta['longitude'],
}
if self._show_on_map:
attr[ATTR_LATITUDE] = self.luftdaten.data.meta['latitude']
attr[ATTR_LONGITUDE] = self.luftdaten.data.meta['longitude']

This comment has been minimized.

Copy link
@fabaff

fabaff Feb 15, 2018

Member

If show_on_map is True then we will have duplicate entries in the state card (lat/ and long/longitude). It should be either lat and long or latitude and longitude.

This comment has been minimized.

Copy link
@c7h

c7h Feb 15, 2018

Author Contributor

OK - didn't know that that's a problem. I thought this way it will be always non-breaking.

This comment has been minimized.

Copy link
@fabaff

fabaff Feb 15, 2018

Member

It's not a problem but looks wired and could confuse people because the same data is shown twice. [ATTR_LATITUDE] and [ATTR_LONGITUDE] are exposed by default. Don't append it to the existing attributes, just make two different dict in regard of the configuration.

This comment has been minimized.

Copy link
@c7h

c7h Feb 15, 2018

Author Contributor

ok, agreed! 👍

@c7h c7h referenced this pull request Feb 15, 2018

Merged

updated documentation: show on map #4680

2 of 2 tasks complete
@c7h

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2018

@fabaff anything else? :)

@fabaff

fabaff approved these changes Mar 6, 2018

Copy link
Member

left a comment

Thanks 🐦

@fabaff fabaff merged commit e6364b4 into home-assistant:dev Mar 6, 2018

5 checks passed

WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.001%) to 94.073%
Details
hound No violations found. Woof!

@balloob balloob referenced this pull request Mar 9, 2018

Merged

0.65 #12995

@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.