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

Update to buienradar json api; and additional monitored_conditions #24463

Merged
merged 17 commits into from Jul 28, 2019

Conversation

@mjj4791
Copy link
Contributor

commented Jun 10, 2019

Breaking Change:

The following sensor types (monitored_conditions) are no longer supported, since they are no longer provided by the json api of buienradar:

  • snow_1d .. snow_5d

The following monitored conditions will change unit:

  • windgust (now km/h, was m/s)
  • windspeed (now km/h, was m/s)
  • windspeed_?d (now km/h, was m/s)
  • visibility (now km, was m)

Description:

Switching over to new version of python-buienrader (1.0.1); this version now leverages the new json buienradar-api. This API provides new sensor data to home-assistant:

  • barometerfc
  • barometerfcname
  • barometerfcnamenl
  • feeltemperature
  • rainlast24hour
  • rainlasthour
  • minrain_1d .. minrain_5d
  • maxrain_1d .. maxrain_5d

Forecast data (Weather component) will now also contain:

  • windbearing
  • windspeed

Related issue (if applicable):
Pyhton-buienradar 1.0.1 now also includes the buienradar radar url functionality, requested here: #23358

unit conversion from m and m/s to km and km/h, as mentioned here:
#20142
#18558

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

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:

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

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

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@mjj4791 mjj4791 referenced this pull request Jun 10, 2019

Merged

updated buienradar.sensor docs #9605

2 of 2 tasks complete

@mjj4791 mjj4791 marked this pull request as ready for review Jun 10, 2019

WINDSPEED)

if not self._forecast:
return

This comment has been minimized.

Copy link
@OttoWinter

OttoWinter Jun 14, 2019

Member
Suggested change
return
return None

Explicit None return if other returns return a value.

data_out = {}
condcode = data_in.get(CONDITION, []).get(CONDCODE)

data_out[ATTR_FORECAST_TIME] = data_in.get(DATETIME)

This comment has been minimized.

Copy link
@OttoWinter

OttoWinter Jun 14, 2019

Member

Could be simplified to

data_out = {
  ATTR_FORECAST_TIME: data_in.get(DATETIME),
  ATTR_FORECAST_CONDITION: cond[condcode],
  # ...
}
@mjj4791

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

I processed the requested review items; not sure what I can do about the conflicting files?

@mjj4791

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

@OttoWinter
Can you be of assistance resolving the conflicts in manifest/codeowners?

Seems we have 2 owners in one component now (@mjj4791 for sensor/weather and @ties for the camera component in buienradar). However I am unable to convince my update/commit to manifest and codeowners to get accepted...

@ties

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

@mjj4791 I think this is similar to what happened when Paulus added me as as a codeowner in the manifest. I think that the linter that checks that that CODEOWNERS is identical to the output generated by the script from the manifest metadata.

I think that is generated by python3 -m script.hassfest

Not on a machine with a dev environment at the moment so I can not check.

@mjj4791

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

I ran python3 -m script.hassfest and updated CODEOWNERS to match;
still no luck getting the file to be accepted....

@ties

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

It seems to pass the checks but it has merge errors. I rebased the branch from dev, skipping empty commits. Doing that and force pushing should make this mergeable.

My result is in https://github.com/ties/home-assistant/tree/feature/pr24463_rebased

edit: to clarify, I will not create a PR for this and can't edit this pr.

If we do documentation changes, it would be nice to include the buienradar camera, turns out I never got that into the documentation (oops).

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

Please rebase on latest dev branch and solve the merge conflicts.

@MartinHjelmare MartinHjelmare added this to Review in progress in Dev Jul 23, 2019

Dev automation moved this from Review in progress to Reviewer approved Jul 23, 2019

@MartinHjelmare
Copy link
Member

left a comment

Can be merged when build passes.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Jul 27, 2019

@mjj4791 please rebase so we can merge this.

@mjj4791 mjj4791 force-pushed the mjj4791:dev branch from 69ae3e9 to c9bb07e Jul 28, 2019

mjj4791 added some commits Jun 10, 2019

Update sensor.py
unit conversion for:
- windgust (ms to km/h)
- windspeed (m/s to km/h)
- windspeed_1d (ms/km/h)
- visibility (m to km)
Update weather.py
unit conversion for windspeed (m/s to km/h)

mjj4791 added some commits Jun 30, 2019

Update CODEOWNERS
yet another try to get CODEOWNERS accepted...

@mjj4791 mjj4791 force-pushed the mjj4791:dev branch from 2812998 to 8ac8ad9 Jul 28, 2019

@mjj4791 mjj4791 force-pushed the mjj4791:dev branch from 8ac8ad9 to 03821cf Jul 28, 2019

@project-bot project-bot bot moved this from Reviewer approved to Needs review in Dev Jul 28, 2019

@home-assistant home-assistant deleted a comment from homeassistant Jul 28, 2019

@home-assistant home-assistant deleted a comment from homeassistant Jul 28, 2019

@home-assistant home-assistant deleted a comment from homeassistant Jul 28, 2019

Dev automation moved this from Needs review to Reviewer approved Jul 28, 2019

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Jul 28, 2019

File CODEOWNERS is not up to date. Please run python3 -m script.hassfest

@MartinHjelmare MartinHjelmare merged commit 14c3b38 into home-assistant:dev Jul 28, 2019

11 checks passed

CI Build #20190728.46 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python35) Tests PyTest Python35 succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing da53e0a...0352165
Details
codecov/project 94.02% (target 90%)
Details

Dev automation moved this from Reviewer approved to Done Jul 28, 2019

@balloob balloob referenced this pull request Aug 7, 2019

Merged

0.97.0 #25756

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