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

hass(improvement): stop specific values from populating #732

Closed
wants to merge 7 commits into from

Conversation

varet80
Copy link
Collaborator

@varet80 varet80 commented Feb 24, 2021

Some values cause too much chatter in Hass, we remove these from discovery populating

@varet80 varet80 requested a review from robertsLando February 24, 2021 14:00
@coveralls
Copy link

coveralls commented Feb 24, 2021

Pull Request Test Coverage Report for Build 596251330

  • 8 of 13 (61.54%) changed or added relevant lines in 1 file are covered.
  • 12 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.02%) to 19.152%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/Gateway.js 8 13 61.54%
Files with Coverage Reduction New Missed Lines %
lib/Gateway.js 12 20.07%
Totals Coverage Status
Change from base Build 596224557: -0.02%
Covered Lines: 2022
Relevant Lines: 10831

💛 - Coveralls

Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

I prefer to move the check in discoverValue function

@varet80
Copy link
Collaborator Author

varet80 commented Feb 24, 2021

wouldn't that iterate all values and just return, instead of stoping earlier?

@robertsLando
Copy link
Member

@billiaz discoverValue could also run 'on fly' and in that case this values will be discovered because you rely on the filter done by the prioritized cc function.

@zwave-js-assistant
Copy link

🚧 It seems like this PR has lint errors 🚧

I should be able to fix them for you. If you want me to, just comment
@zwave-js-bot fix lint

@varet80
Copy link
Collaborator Author

varet80 commented Feb 24, 2021

@zwave-js-bot fix lint

Comment on lines +1612 to +1619
// List of values
const ignoreDiscovery = {
Meter: ['previousValue', 'deltaTime']
}
if (ignoreDiscovery[valueId.commandClassName]) {
if (ignoreDiscovery[valueId.commandClassName].includes(valueId.property))
return
}
Copy link
Member

Choose a reason for hiding this comment

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

I like this but for now there are much other cc we are ignoring, IMO this check should be done inside the CC switch-case, like if(property === x || property === y) return

Copy link
Contributor

Choose a reason for hiding this comment

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

thought of. looking if we have second class, otherwise i will move it under. the idea was to cover all

@robertsLando
Copy link
Member

With zwavejs 7 deltaTime and previous value will not be available anymore so this can be closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants