Skip to content

Move CONF_UPDATE_INTERVAL to homeassistant.const#20526

Merged
balloob merged 1 commit into
home-assistant:devfrom
rohankapoorcom:conf-update-interval
Jan 29, 2019
Merged

Move CONF_UPDATE_INTERVAL to homeassistant.const#20526
balloob merged 1 commit into
home-assistant:devfrom
rohankapoorcom:conf-update-interval

Conversation

@rohankapoorcom
Copy link
Copy Markdown
Member

Description:

CONF_UPDATE_INTERVAL is used by a lot of components and platforms and redefined in each module. We should pull it up one level to homeassistant.const and use it from there instead.

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

@arsaboo
Copy link
Copy Markdown
Contributor

arsaboo commented Jan 28, 2019

So, just for my clarification, what is the difference between CONF_UPDATE_INTERVAL and CONF_SCAN_INTERVAL that many platforms use?

@MartinHjelmare
Copy link
Copy Markdown
Member

That's a good question. We don't really need both I think. Problem is that it would be a breaking change to remove either. @fabaff what do you think?

@arsaboo
Copy link
Copy Markdown
Contributor

arsaboo commented Jan 28, 2019

A cursory look suggests that CONF_SCAN_INTERVAL is significantly more popular (by almost a factor of 5).

@rohankapoorcom
Copy link
Copy Markdown
Member Author

rohankapoorcom commented Jan 28, 2019

I think if we are going to do a breaking change, we should do it for all components using CONF_UPDATE_INTERVAL at once. One possible approach, add a voluptuous validator wherever it's used and mark the argument as "Deprecated, replace with scan_interval. This will be invalid in release 0.xx". Where we pick a release (maybe 4 versions out) where it will be hard removed. Of course, we'll have to configure the component to use both options during that time period, but I think it's a better approach than a hard breaking change just dropped in the next release.

It looks we have a deprecated helper already, but it doesn't indicate what the config should be replaced with. That should be an easy change to make though

I'll update the PR with this approach if that makes sense.

@balloob balloob merged commit cc74035 into home-assistant:dev Jan 29, 2019
@ghost ghost removed the in progress label Jan 29, 2019
@balloob
Copy link
Copy Markdown
Member

balloob commented Jan 29, 2019

So let's merge this as is, it's a good change. A breaking change can happen in a future PR.

However, after the entity ID breaking change debacle, I prefer to not do breaking changes if we can avoid it. I rather have us standardize it when we move to config entries and leave config yaml as is.

@rohankapoorcom rohankapoorcom deleted the conf-update-interval branch January 29, 2019 04:46
@rohankapoorcom
Copy link
Copy Markdown
Member Author

@balloob I agree, avoiding breaking changes is good. Let me push up what I have for doing a soft deprecated and then hard deprecation after a set number of versions and see if that makes sense.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants