Python spotcrime #12460
Python spotcrime #12460
Conversation
|
Hi @jcconnell, It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
homeassistant
added
cla-needed
core
new-platform
platform: sensor.spotcrime
cla-signed
and removed
cla-needed
labels
|
Neat! Is this ultimately the same data as |
|
Hi! I used your code as a template since this is my first time contributing (both to HASS and OSS). I used crimereports in my HA installation for a while, but never received any notice of an incident. After digging in a bit further, I realized that Crime Reports doesn't receive any notifications in my area, but Spot Crime does. I don't know of their specific differences across the states but in my area, Spot Crime provides some results where Crime Reports didn't. |
|
@happyleavesaoc Thank you for the help! I'm not very familiar with this process. What's the best way to remove the file from the PR? |
|
homeassistant/const.py
Outdated
| @@ -51,6 +51,7 @@ | |||
| CONF_CUSTOMIZE = 'customize' | |||
| CONF_CUSTOMIZE_DOMAIN = 'customize_domain' | |||
| CONF_CUSTOMIZE_GLOB = 'customize_glob' | |||
| CONF_DAYS = 'days' | |||
There was a problem hiding this comment.
Do that only if you can replace it and use it on other components/platform
There was a problem hiding this comment.
What should I remove or how should I change it to be correct?
There was a problem hiding this comment.
I think @pvizeli is saying that const.py is for constants that are broadly applicable; as far as we know, CONF_DAYS is only useful to your platform at the moment. So, until it's determined otherwise, it's best placed within spotcrime.py.
There was a problem hiding this comment.
Thank you @bachya, I see now. I did not understand well enough how to bring in the configuration variables. Some of the information at this link gave me a better idea: https://community.home-assistant.io/t/help-using-config-get-for-custom-platform-switch/4092/2. I am working to do it the correct way now.
| CONF_DAYS = 'days' | ||
| DEFAULT_DAYS = 1 | ||
|
|
||
| DOMAIN = 'spotcrime' |
There was a problem hiding this comment.
This platform is part of the sensor domain. So don't overwrite that. If you need a constant to be used multiple times, just rename it to something else.
| """Set up the Crime Reports platform.""" | ||
| latitude = config.get(CONF_LATITUDE, hass.config.latitude) | ||
| longitude = config.get(CONF_LONGITUDE, hass.config.longitude) | ||
| name = config.get(CONF_NAME) |
There was a problem hiding this comment.
The name is required in the config schema, so don't use dict.get. Do:
name = config[CONF_NAME]| latitude = config.get(CONF_LATITUDE, hass.config.latitude) | ||
| longitude = config.get(CONF_LONGITUDE, hass.config.longitude) | ||
| name = config.get(CONF_NAME) | ||
| radius = config.get(CONF_RADIUS) |
| class SpotCrimeSensor(Entity): | ||
| """Representation of a Spot Crime Sensor.""" | ||
|
|
||
| def __init__(self, hass, name, latitude, longitude, radius, |
There was a problem hiding this comment.
Don't pass in hass. It will be set on the entity when it has been added to home assistant.
| include, exclude, days): | ||
| """Initialize the Spot Crime sensor.""" | ||
| import spotcrime | ||
| self._hass = hass |
| ATTR_LATITUDE: incident.get('lat'), | ||
| ATTR_LONGITUDE: incident.get('lon') | ||
| }) | ||
| self._hass.bus.fire(EVENT_INCIDENT, data) |
| import spotcrime | ||
| incident_counts = defaultdict(int) | ||
| incidents = self._spotcrime.get_incidents() | ||
| fire_events = len(self._previous_incidents) > 0 |
There was a problem hiding this comment.
You don't need to define a new variable here. Just check if self._previous_incidents is truthy below.
| for incident in incidents: | ||
| incident_type = slugify(incident.get('type')) | ||
| incident_counts[incident_type] += 1 | ||
| if (fire_events and incident.get('id') |
There was a problem hiding this comment.
if (self._previous_incidents and incident.get('id')
not in self._previous_incidents):| self._incident_event(incident) | ||
| self._previous_incidents.add(incident.get('id')) | ||
| self._attributes = { | ||
| ATTR_ATTRIBUTION: spotcrime.ATTRIBUTION |
There was a problem hiding this comment.
Do this in init instead.
|
|
||
| def update(self): | ||
| """Update device state.""" | ||
| import spotcrime |
|
Fix the lint issue and we can merge. |
|
@MartinHjelmare Thank you for the review! Really appreciate the detail. Removed the un-needed import and I think I fixed the lint issue. Testing now. |
|
Nice! Thanks for all the help on my first go everyone! |
ghost
added
Description:
Added support for the Spot Crime API.
Related issue (if applicable): fixes #
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4685
Example entry for
configuration.yaml(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
toxrun successfully. Your PR cannot be merged unless tests passREQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.If the code does not interact with devices:
toxrun successfully. Your PR cannot be merged unless tests pass