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

Python spotcrime #12460

Merged
merged 8 commits into from Feb 22, 2018

Conversation

@jcconnell
Copy link
Contributor

commented Feb 16, 2018

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):

sensor:
  - platform: spotcrime
     name: Spot Crime
     days: 8
     radius: 0.05 # 5 miles

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.

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

jcconnell added some commits Feb 16, 2018

@jcconnell jcconnell requested review from andrey-git and home-assistant/core as code owners Feb 16, 2018

@homeassistant

This comment has been minimized.

Copy link

commented Feb 16, 2018

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!

@happyleavesaoc

This comment has been minimized.

Copy link
Collaborator

commented Feb 16, 2018

Neat! Is this ultimately the same data as crimereports, or is there some other advantage?

@jcconnell

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2018

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
Copy link
Collaborator

left a comment

You'll want to remove man/man1/nosetests.1 from the PR. I think it got accidently included.

@jcconnell

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2018

@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?

@balloob

This comment has been minimized.

Copy link
Member

commented Feb 16, 2018

git rm man/man1/nosetests.1
git commit -m "Remove accidental included file"
git push
@@ -51,6 +51,7 @@
CONF_CUSTOMIZE = 'customize'
CONF_CUSTOMIZE_DOMAIN = 'customize_domain'
CONF_CUSTOMIZE_GLOB = 'customize_glob'
CONF_DAYS = 'days'

This comment has been minimized.

Copy link
@pvizeli

pvizeli Feb 16, 2018

Member

Do that only if you can replace it and use it on other components/platform

This comment has been minimized.

Copy link
@jcconnell

jcconnell Feb 16, 2018

Author Contributor

What should I remove or how should I change it to be correct?

This comment has been minimized.

Copy link
@bachya

bachya Feb 16, 2018

Contributor

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.

This comment has been minimized.

Copy link
@jcconnell

jcconnell Feb 19, 2018

Author Contributor

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.

jcconnell added some commits Feb 19, 2018

CONF_DAYS = 'days'
DEFAULT_DAYS = 1

DOMAIN = 'spotcrime'

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 21, 2018

Member

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)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 21, 2018

Member

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)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 21, 2018

Member

Same as above.

class SpotCrimeSensor(Entity):
"""Representation of a Spot Crime Sensor."""

def __init__(self, hass, name, latitude, longitude, radius,

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 21, 2018

Member

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

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 21, 2018

Member

Remove this.

ATTR_LATITUDE: incident.get('lat'),
ATTR_LONGITUDE: incident.get('lon')
})
self._hass.bus.fire(EVENT_INCIDENT, data)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 21, 2018

Member

Use self.hass.

import spotcrime
incident_counts = defaultdict(int)
incidents = self._spotcrime.get_incidents()
fire_events = len(self._previous_incidents) > 0

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 21, 2018

Member

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')

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 21, 2018

Member
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

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 21, 2018

Member

Do this in init instead.


def update(self):
"""Update device state."""
import spotcrime

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 21, 2018

'spotcrime' imported but unused

@MartinHjelmare
Copy link
Member

left a comment

Looks good!

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Feb 21, 2018

Fix the lint issue and we can merge.

@jcconnell

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2018

@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.

@balloob balloob merged commit 4fdbbc4 into home-assistant:dev Feb 22, 2018

4 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
coverage/coveralls Coverage decreased (-0.03%) to 94.05%
Details
hound No violations found. Woof!
@jcconnell

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2018

Nice! Thanks for all the help on my first go everyone!

@balloob balloob referenced this pull request Feb 22, 2018

Merged

0.64.0 #12609

@home-assistant home-assistant locked and limited conversation to collaborators May 29, 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.