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

Add sensor.nsw_fuel_station component #14757

Merged
merged 16 commits into from Jun 14, 2018

Conversation

Projects
None yet
7 participants
@nickw444
Contributor

nickw444 commented Jun 2, 2018

Description:

Adds a new component, sensor.nsw_fuel_station to show fuel prices from fuel stations in NSW, Australia.

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

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: nsw_fuel_station
    station_name: 'Caltex Woolworths Blakehurst'
    station_id: 530
    fuel_types:
      - E10
      - P95
  - platform: nsw_fuel_station
    station_id: 675
    station_name: Caltex Woolworths Peakhurst
    fuel_types:
      - E10
      - P95

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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

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

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated 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:

  • Tests have been added to verify that the new code works.
@property
def name(self) -> str:
"""Return the name of the sensor."""
return 'NSW Fuel Station {} {}'.format(self._station_data.station_name, self._fuel_type)

This comment has been minimized.

@houndci-bot

houndci-bot Jun 2, 2018

line too long (96 > 79 characters)

station_data = StationPriceData(client, station_id, station_name)
station_data.update()
add_devices([StationPriceSensor(station_data, fuel_type) for fuel_type in fuel_types])

This comment has been minimized.

@houndci-bot

houndci-bot Jun 2, 2018

line too long (90 > 79 characters)

PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Required(CONF_STATION_ID): cv.positive_int,
vol.Required(CONF_STATION_NAME): cv.string,
vol.Required(CONF_FUEL_TYPES, default=[]): vol.All(cv.ensure_list, [cv.string]),

This comment has been minimized.

@houndci-bot

houndci-bot Jun 2, 2018

line too long (84 > 79 characters)

@nickw444 nickw444 referenced this pull request Jun 2, 2018

Merged

NSW Fuel Station Price Sensor #5477

2 of 2 tasks complete
PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Required(CONF_STATION_ID): cv.positive_int,
vol.Required(CONF_STATION_NAME): cv.string,
vol.Required(CONF_FUEL_TYPES, default=[]): vol.All(

This comment has been minimized.

@dgomes

dgomes Jun 2, 2018

Member

if it is required there is no point in having a default, maybe make it option and have the default a non empty list ?

This comment has been minimized.

@nickw444

nickw444 Jun 3, 2018

Contributor

Great suggestion - I have changed the default to now be the full set of available fuel types and have updated the platform setup to only create sensors where there is an available fuel type to match in the station data (some stations don't supply every type of fuel)

"""Return the price of the given fuel type."""
if self._data is None:
return None
return next((x for x in self._data if x.fuel_type == fuel_type), None)

This comment has been minimized.

@dgomes

dgomes Jun 2, 2018

Member

maybe renaming x to price ?

nickw444 added some commits Jun 3, 2018

@dgomes

This comment has been minimized.

Member

dgomes commented Jun 3, 2018

Can you work the test to use MockDependency ?

This would enable for the removal of the dependency from requirements_test_all.txt

@dgomes

This comment has been minimized.

Member

dgomes commented Jun 3, 2018

I think it is good to go!

Just remove your dependency from script/gen_requirements_all.py and requirements_test_all.txt

@nickw444

This comment has been minimized.

Contributor

nickw444 commented Jun 4, 2018

Looks like the tests are failing due to the missing dependency, since setup_platform does from nsw_fuel import FuelCheckClient.

Is there a proper way for me to patch the module so that the tests will still run even if the package isn't installed.?

@dgomes

This comment has been minimized.

Member

dgomes commented Jun 4, 2018

That's where MockDependency I mentioned you some comments above comes in.

Just look into other tests that already use MockDependency

from homeassistant.components import sensor
from homeassistant.setup import setup_component
from tests.common import get_test_home_assistant, assert_setup_component, MockDependency

This comment has been minimized.

@houndci-bot

houndci-bot Jun 4, 2018

line too long (88 > 79 characters)

@nickw444

This comment has been minimized.

Contributor

nickw444 commented Jun 4, 2018

Ah perfect, I must have overlooked that. Thanks for the review 👍

@dgomes

dgomes approved these changes Jun 7, 2018

LGTM

Component to display the current fuel prices at a NSW fuel station.
For more details about this component, please refer to the documentation at
https://home-assistant.io/components/nsw_fuel_station/

This comment has been minimized.

@fabaff

fabaff Jun 7, 2018

Member

URL is wrong. Should be https://home-assistant.io/components/sensor.nsw_fuel_station/.

"""
Component to display the current fuel prices at a NSW fuel station.
For more details about this component, please refer to the documentation at

This comment has been minimized.

@fabaff

fabaff Jun 7, 2018

Member

It's a platform and not a component. nsw_fuel_station is a sensor platform.

"""
import datetime
from typing import Optional
import voluptuous as vol

This comment has been minimized.

@fabaff
def setup_platform(hass, config, add_devices, discovery_info=None):
"""Set up the NSW Fuel Station component."""

This comment has been minimized.

@fabaff

fabaff Jun 7, 2018

Member

Use sensor instead of component please.

PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Required(CONF_STATION_ID): cv.positive_int,
vol.Required(CONF_STATION_NAME): cv.string,

This comment has been minimized.

@fabaff

fabaff Jun 7, 2018

Member

Must this really be required. station_id is used to retrieve the data incl. the station name. Best would be to let the users set their own name (CONF_NAME) and fall-back to the name which the request returns.

This comment has been minimized.

@nickw444

nickw444 Jun 9, 2018

Contributor

Unfortunately the response from the API does not return the fuel station name:

https://api.onegov.nsw.gov.au/FuelCheckApp/v1/fuel/prices/station/2119

An alternative to this could be to just use the station id as the component name? What do you think is a better user experience?

This comment has been minimized.

@fabaff

fabaff Jun 9, 2018

Member

Hmm, I just checked one station from https://www.fuelcheck.nsw.gov.au and the name seems to be there.

[{
  "ServiceStationID":1706,
  "Name":"BP Edgecliff",
  "Lat":-33.876974,
  "Long":151.231099,
[...]

Let the users have the option to name a sensor according their needs in the configuration will avoid that they have to use customize:. I personally prefer if the default name let me identify the sensor quickly, thus I would just use Fuel Price + fuel type. Station Id is not that useful, IMHO.

This comment has been minimized.

@nickw444

nickw444 Jun 9, 2018

Contributor

Gotcha, I thought you were implying I obtained the station name by using the same request/endpoint. I will update the setup_platform to make an addition request during setup to find the name of the station using another endpoint. Definitely makes sense to make this easier to configure

Let the users have the option to name a sensor according their needs in the configuration will avoid that they have to use customize:

I'll remove the need for a name in the config entirely

thus I would just use Fuel Price + fuel type. Station Id is not that useful, IMHO.

Just double checking, I think you mean Name + fuel type?

for fuel_type in fuel_types
if fuel_type in available_fuel_types
])
return True

This comment has been minimized.

@fabaff

fabaff Jun 7, 2018

Member

Not needed.

def __init__(self, client, station_id: int, station_name: str) -> None:
"""Initialize the sensor."""
self.station_id = station_id
self.station_name = station_name

This comment has been minimized.

@fabaff

fabaff Jun 7, 2018

Member

Not needed. Thus no need to pass it.

@property
def name(self) -> str:
"""Return the name of the sensor."""
return 'NSW Fuel Station {} {}'.format(

This comment has been minimized.

@fabaff

fabaff Jun 7, 2018

Member

Remove NSW Fuel Station because this will create very long identifier and makes it not very handy to use in templates. Limit it to station name and fuel type. With CONF_NAME can the still add NSW Fuel Station if needed.

Entity ID: sensor.nsw_fuel_station_caltex_woolworths_peak
Friendly name: NSW Fuel Station Caltex Woolworths Peakhurst P95

"""Return the state attributes of the device."""
attr = {}
attr['Station ID'] = self._station_data.station_id
attr['Station Name'] = self._station_data.station_name

This comment has been minimized.

@fabaff

fabaff Jun 7, 2018

Member

Please add an attribution for the source of the data.

fuel_types = config[CONF_FUEL_TYPES]
client = FuelCheckClient()
station_data = StationPriceData(client, station_id, station_name)

This comment has been minimized.

@fabaff

fabaff Jun 7, 2018

Member

The setup of the platform should fail if the station ID is wrong/not available and an error logged (log file and/or persistent notification). Otherwise will be users end up with a non-functional platform.

This comment has been minimized.

@nickw444

nickw444 Jun 9, 2018

Contributor

Added persistent notification with error message:

screen shot 2018-06-09 at 9 37 37 pm

"""Return the state of the sensor."""
price_info = self._station_data.for_fuel_type(self._fuel_type)
if price_info:
return price_info.price

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jun 7, 2018

Member

Please return None explicitly if no price is found.

This comment has been minimized.

@nickw444

nickw444 Jun 9, 2018

Contributor

Fixed!

@fabaff

This comment has been minimized.

Member

fabaff commented Jun 7, 2018

Also, nickw444/nsw-fuel-api-client#1 should be addressed and the requirements updated.

wip

@wafflebot wafflebot bot added the in progress label Jun 9, 2018

message,
title=NOTIFICATION_TITLE,
notification_id=NOTIFICATION_ID)
return False

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jun 10, 2018

Member

Just return.

self.station_id)
except FuelCheckError as exc:
self.error = str(exc)
_LOGGER.exception(

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jun 10, 2018

Member

Use error level.

nickw444 added some commits Jun 10, 2018

except FuelCheckError as exc:
self.error = str(exc)
_LOGGER.error(
'Failed to fetch NSW Fuel station reference data', exc_info=exc)

This comment has been minimized.

@houndci-bot

houndci-bot Jun 11, 2018

line too long (84 > 79 characters)

nickw444 added some commits Jun 11, 2018

self.error = str(exc)
_LOGGER.error(
'Failed to fetch NSW Fuel station reference data',
exc_info=exc)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jun 11, 2018

Member

Just pass the exception as a string formatting parameter to the logging string message. Don't use exc_info kwarg.

_LOGGER.error("bla bla %s", exc)
except FuelCheckError as exc:
self.error = str(exc)
_LOGGER.error(
'Failed to fetch NSW Fuel station price data', exc_info=exc)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jun 11, 2018

Member

See above.

@MartinHjelmare

Looks good!

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Jun 11, 2018

@fabaff should approve. But looks mergeable when build passes.

@fabaff

fabaff approved these changes Jun 14, 2018

@fabaff fabaff merged commit cdd111d into home-assistant:dev Jun 14, 2018

4 of 5 checks passed

WIP work in progress
Details
Hound No violations found. Woof!
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 94.001%
Details

@wafflebot wafflebot bot removed the in progress label Jun 14, 2018

@DavidFW1960 DavidFW1960 referenced this pull request Jun 20, 2018

Closed

NSW Fuel Sensor #5555

@balloob balloob referenced this pull request Jun 22, 2018

Merged

0.72 #15088

MizterB added a commit to MizterB/home-assistant that referenced this pull request Aug 11, 2018

Add sensor.nsw_fuel_station component (home-assistant#14757)
* Add sensor.nsw_fuel_station component

* bump dependency

* PR Changes

* flake8

* Use MockPrice

* Fix requirements

* Fix tests

* line length

* wip

* Handle errors and show persistent notification

* update tests

* Address @MartinHjelmare's comments

* Fetch station name from API

* Update tests

* Update requirements

* Address comments

cyberjacob pushed a commit to cyberjacob/home-assistant that referenced this pull request Sep 4, 2018

Add sensor.nsw_fuel_station component (home-assistant#14757)
* Add sensor.nsw_fuel_station component

* bump dependency

* PR Changes

* flake8

* Use MockPrice

* Fix requirements

* Fix tests

* line length

* wip

* Handle errors and show persistent notification

* update tests

* Address @MartinHjelmare's comments

* Fetch station name from API

* Update tests

* Update requirements

* Address comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment