Skip to content

Add offset option to sensor.gtfs#7980

Merged
fabaff merged 3 commits into
home-assistant:devfrom
Kernald:dev
Jun 24, 2017
Merged

Add offset option to sensor.gtfs#7980
fabaff merged 3 commits into
home-assistant:devfrom
Kernald:dev

Conversation

@Kernald
Copy link
Copy Markdown
Contributor

@Kernald Kernald commented Jun 9, 2017

Description:

The rationale behind this addition is this: if you have a 10 minutes walk to your bus stop, you're not interested in knowing the next bus is in 4 minutes. Instead, you're interested in knowing that the next one is in 12 minutes, so you'd better go and catch it.
The offset option is meant to do this: add a duration, and all departures before this duration will be ignored.

Note that I know this PR isn't ready for merge. It's my first PR in here, and I never wrote Python before, things are probably wrong. I'm not so sure about the name offset, for a start. I'll update the documentation once everything else is fixed in this PR.

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

Example entry for configuration.yaml (if applicable):

- platform: gtfs
  name: tisseo_home_af
  origin: !secret tisseo_stop_home
  destination: !secret tisseo_stop_af_arrival
  offset:
    minutes: 16
  data: tisseo_gtfs.zip

Checklist:

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.

Comment thread homeassistant/components/sensor/gtfs.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (82 > 79 characters)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

About that, should I import datetime.timedelta at the top of the file, or go ahead and split this line?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add an enter after :

Comment thread homeassistant/components/sensor/gtfs.py Outdated
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Like I said, it's the first time I ever wrote Python. I'm not sure what's @property meant for. I expected the offset to show up in the sensor attributes (on the web UI), but it doesn't. Is this property mandatory?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use device_state_attributes() to collect and present attributes in the frontend.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This component actually already implements device_state_attributes, which returns self._attributes. self._attributes is updated inside update, so just make sure you set the attribute there.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That being said, we can also just remove it. Maybe not so important to have it in the attributes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Having it in the attributes might be useful for some notifications or that kind of things. I'll fix this, thanks for the pointers!

Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

Looks good. Ok to merge when line length issue addressed and property either added to self._attributes or removed.

@homeassistant
Copy link
Copy Markdown
Contributor

Hi @Kernald,

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!

Comment thread homeassistant/components/sensor/gtfs.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

continuation line unaligned for hanging indent

Copy link
Copy Markdown
Member

@fabaff fabaff left a comment

Choose a reason for hiding this comment

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

All comments addressed.

@fabaff fabaff merged commit edeb92e into home-assistant:dev Jun 24, 2017
fabaff pushed a commit to home-assistant/home-assistant.io that referenced this pull request Jun 24, 2017
* Add documentation for sensor.gtfs offset option

Added in PR home-assistant/core#7980

* Add default
@balloob balloob mentioned this pull request Jul 1, 2017
dethpickle pushed a commit to dethpickle/home-assistant that referenced this pull request Aug 18, 2017
* Add offset option to sensor.gtfs

* Fix long lines in sensor.gtfs

* Expose GTFS offset as an attribute
@home-assistant home-assistant locked and limited conversation to collaborators Oct 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants