Skip to content

Conversation

MrHarcombe
Copy link
Contributor

@MrHarcombe MrHarcombe commented Apr 29, 2020

Breaking change

As this moves the config for the MetOffice weather integration into the UI from the YAML, you will need to ensure you keep a copy of the DataPoint API key handy to re-enable the integration in your server. The location being forecast is, by default, taken as the location of the server, so again if you were forecasting other locations you will need to have those GPS co-ordinates to hand, to re-enter them when configuring new integrations through the UI.

Proposed change

In line with the request made following PR 34642 (link below), this change moves the MetOffice weather integration away from YAML to use the UI (config_flow) configuration.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold - I think?
  • 🏆 Platinum

- Sensor and Weather platforms now use config_flow with unique_ids
- MetOfficeData object is now async-protected
- Integration uses DataUpdateCoordinator and so refreshes correctly
- Test cases written to test config_flow and entity behaviours
@MartinHjelmare MartinHjelmare changed the title Converted MetOffice to use UI for configuration Convert MetOffice to use UI for configuration Apr 30, 2020
Copy link
Contributor

@MatthewFlamm MatthewFlamm left a comment

Choose a reason for hiding this comment

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

FYI, these comments are based on my experience in writing my own weather integration with config flow. I do not claim an authoritative understanding.

MrHarcombe added 2 commits May 1, 2020 15:02
- Not all sensor entities are now enabled by default
- Changes to the test cases to only therefore test sensor entities that *are* enabled
- Extend test coverage by adding a case that interrupts connection
@MatthewFlamm
Copy link
Contributor

I haven't looked in detail at your tests, but it seems you are testing all the way down to the platforms and entities. You can probably remove those files from .coveragerc if so.

@MrHarcombe
Copy link
Contributor Author

I haven't looked in detail at your tests, but it seems you are testing all the way down to the platforms and entities. You can probably remove those files from .coveragerc if so.

I am, yes. I had great fun getting to grips with mock requests and fixed date times, to ensure that the data always matched :) - I'll update .coveragerc as you suggest.

@stale
Copy link

stale bot commented Jun 3, 2020

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale label Jun 3, 2020
@MrHarcombe
Copy link
Contributor Author

MrHarcombe commented Jun 3, 2020 via email

@stale stale bot removed the stale label Jun 3, 2020
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Mostly just some clean up needed. Some changes around sensor with device classes though.

- Sensor and Weather platforms now use config_flow with unique_ids
- MetOfficeData object is now async-protected
- Integration uses DataUpdateCoordinator and so refreshes correctly
- Test cases written to test config_flow and entity behaviours
…gative cases, as well as the positive cases.
- Not all sensor entities are now enabled by default
- Changes to the test cases to only therefore test sensor entities that *are* enabled
- Extend test coverage by adding a case that interrupts connection
@MartinHjelmare
Copy link
Member

Did you commit changes without pushing them? Many of my comments have been resolved but have not been addressed in the code.

@MrHarcombe
Copy link
Contributor Author

Did you commit changes without pushing them? Many of my comments have been resolved but have not been addressed in the code.

D'oh - on it.

Found an example to work from, then just had to realise to add a proper unique_id for the test location, so the next entry would be refused.
MrHarcombe and others added 3 commits June 12, 2020 18:00
As per review suggestion

Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
As per review suggestion

Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Serves me right for not going through the proper steps...
@MartinHjelmare MartinHjelmare self-assigned this Jun 12, 2020
With pointers from @MartinHjelmare, finally found the right way to patching datetime.now so as to be able to freeze time for datapoint API to return known weather data for testing.
Thanks to @MartinHjelmare (again) for this very neat way of Mock'ing into datetime.now directly on the patch declaration.
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Great!

@MartinHjelmare
Copy link
Member

Please add a breaking change paragraph where we briefly describe what changed and what the user needs to do to cope with the breaking change.

@MrHarcombe
Copy link
Contributor Author

Please add a breaking change paragraph where we briefly describe what changed and what the user needs to do to cope with the breaking change.

Done. Thank you!

(How do I go about un-"draft"ing the dependent PR that adds forecast data and daily update mode? Presumably I start by merging in all these updates, then...?)

@MartinHjelmare
Copy link
Member

Yeah, rebase on dev branch after this PR has been merged.

@MartinHjelmare MartinHjelmare merged commit c96458c into home-assistant:dev Jun 15, 2020
@MrHarcombe MrHarcombe deleted the metoffice_config_flow branch June 15, 2020 10:05
@tiems90
Copy link

tiems90 commented Jul 8, 2020

A question here, since I haven't been able to find the answer in the documents. The documents mention the entities that are enabled by default; but how do I enable additional non-default endpoints?

Specifically I am looking for visibility_distance and humidity. Previously we set these up under monitored_conditions, but I cannot find where to set this.

@MrHarcombe
Copy link
Contributor Author

MrHarcombe commented Jul 8, 2020 via email

@tiems90
Copy link

tiems90 commented Jul 8, 2020 via email

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Jul 9, 2020
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