Skip to content

Add belgian meter and rename some dsmr sensors#30121

Merged
pvizeli merged 7 commits into
home-assistant:devfrom
dupondje:dsmr_updates
Feb 5, 2020
Merged

Add belgian meter and rename some dsmr sensors#30121
pvizeli merged 7 commits into
home-assistant:devfrom
dupondje:dsmr_updates

Conversation

@dupondje
Copy link
Copy Markdown
Contributor

Breaking Change:

Some sensors were renamed. This because the suffix _low/_normal isn't valid in all cases.
Also DMSR Specs use Tarif 1 and Tarif 2.
Also fix Bug #24075

Description:

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#11509

Example entry for configuration.yaml (if applicable):

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.
  • I have followed the development checklist

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

@homeassistant
Copy link
Copy Markdown
Contributor

Hi @dupondje,

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!

Copy link
Copy Markdown
Member

@springstan springstan left a comment

Choose a reason for hiding this comment

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

Reviewed your PR and found some small stuff, could you please view my comments? Thanks :)

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

@springstan springstan 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 to me 👍

@springstan
Copy link
Copy Markdown
Member

@dupondje you will have to add some tests for the added functionality and a breaking change paragraph :)

@dupondje
Copy link
Copy Markdown
Contributor Author

@dupondje you will have to add some tests for the added functionality and a breaking change paragraph :)

Where to add that breaking change paragraph? :)

@springstan
Copy link
Copy Markdown
Member

In the breaking change section of your PR (the filled out PR template)

@dupondje
Copy link
Copy Markdown
Contributor Author

Should be all fine now :)

@springstan
Copy link
Copy Markdown
Member

@dupondje Looks good but it seems that you will need to add another test to pass the code coverage test :)

@dupondje
Copy link
Copy Markdown
Contributor Author

Added some more tests :) Are we fine now? :)

@MartinHjelmare MartinHjelmare changed the title Add support for belgian meter and rename some sensors Add belgian meter and rename some dsmr sensors Dec 24, 2019
@springstan
Copy link
Copy Markdown
Member

Unfortunately not, could you take a look at the codecoverage and adjust your tests respectively? :)

@dupondje
Copy link
Copy Markdown
Contributor Author

Better now 👯‍♂️

Comment thread homeassistant/components/dsmr/sensor.py Outdated
Comment thread homeassistant/components/dsmr/sensor.py Outdated
@dupondje dupondje requested a review from fabaff January 5, 2020 08:09
@springstan springstan added the dependency Pull requests marked as a dependency upgrade label Jan 13, 2020
@cereal2nd
Copy link
Copy Markdown
Contributor

can we get this one merged now?
as i'm waiting for this change

@frenck
Copy link
Copy Markdown
Member

frenck commented Jan 28, 2020

We can't actually, dsmr_parser, the dependency used by this integration, is currently broken. This is caused by a dependency of dsmr_parser that has been removed from PyPi a couple of days ago.

The upstream library needs to be fixed, because we can consider merging this PR.

@dupondje dupondje requested review from a team and Quentame as code owners January 28, 2020 18:54
@springstan springstan added the waiting-for-upstream We're waiting for a change upstream label Feb 2, 2020
@balloob balloob removed the waiting-for-upstream We're waiting for a change upstream label Feb 5, 2020
@pvizeli pvizeli merged commit 557f576 into home-assistant:dev Feb 5, 2020
@lock lock Bot locked and limited conversation to collaborators Feb 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

breaking-change cla-signed dependency Pull requests marked as a dependency upgrade integration: dsmr small-pr PRs with less than 30 lines.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants