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

Don't expose services in Utility_Meter unless tariffs are available #20878

Merged
merged 2 commits into from Feb 17, 2019

Conversation

@dgomes
Copy link
Contributor

dgomes commented Feb 8, 2019

Description:

The utility_meter component provides services to change tariffs. When there are no tariffs the services fill no purpose. This PR setup's services ONLY when tariffs are defined.

Related issue (if applicable): fixes #20982 and https://community.home-assistant.io/t/utility-meter-reset-service-seems-not-be-working/98271

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>

Example entry for configuration.yaml (if applicable):

utility_meter:
  hourly_energy:
    source: sensor.total_energy_spent
    cycle: hourly

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.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@awarecan
Copy link
Contributor

awarecan left a comment

I would suggest the logic change to:

async def async_setup(hass, config):
    tariffs_are_defined = False
    for meter, conf in config.get(DOMAIN).items():
        if not conf[CONF_TARIFFS]:
            ...
            # prefer guard style over if-else
            continue
        
        tariffs_are_defined = True
        # create tariff selection
        ...
    
    if not tariffs_are_defined:
        return True

    # register services
    ...
    return True
@dgomes

This comment has been minimized.

Copy link
Contributor Author

dgomes commented Feb 9, 2019

@awarecan thank you for your suggestion, but I actually prefer to avoid extra variables (tariffs_are_defined) and the extra if.

@balloob balloob removed this from the 0.87.1 milestone Feb 9, 2019

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Feb 9, 2019

@awarecan is right. Now you will be re-defining the service multiple times.

Removing this from the milestone as this is not fixing something that is broken, it's a nuisance.

@dgomes

This comment has been minimized.

Copy link
Contributor Author

dgomes commented Feb 9, 2019

I didn't realize that 😣 sorry @awarecan

I think it deserves 0.87.1 because I got reports the services were not working (because they shouldn't)

I'll fixe it as soon as I get to a computer

@balloob balloob merged commit 9cab597 into home-assistant:dev Feb 17, 2019

5 checks passed

Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.003%) to 93.357%
Details

@wafflebot wafflebot bot removed the Ready for review label Feb 17, 2019

balloob added a commit that referenced this pull request Feb 17, 2019

Don't expose services in Utility_Meter unless tariffs are available (#…
…20878)

* only expose services when tariffs configured

* don't register services multiple times

@balloob balloob referenced this pull request Feb 20, 2019

Merged

0.88.0 #21238

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.